-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new IntegrationTest annotation #13249
Add new IntegrationTest annotation #13249
Conversation
This composite annotation is meant to abstract complexity of common Integration test annotations in a single file. This commit also adds the annotation to required integration test files. Fix jhipster#12460
Move AbstractNeo4jIT and ReactiveSqlTestContainerExtension to base composite annotation Fix jhipster#12460
Thanks @swarajsaaj I'll review it soon ! |
generators/server/files.js
Outdated
templates: [ | ||
{ | ||
file: 'package/IntegrationTest.java', | ||
renameTo: generator => `${generator.testDir}/${generator.mainClass}IntegrationTest.java`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about my comment, but I think it's better to have simply IntegrationTest, as some project can have very long name, and this annotation will be so long.
What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pascalgrimaud Yes I agree annotation will be quite long for some (for e.g. @LibraryManagementSystemIntegrationTest
is quite long), and package name provides enough namespace info already (com.librarymanagement.app.IntegrationTest
) , so we can go with just @IntegrationTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then, let's change to @IntegrationTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pascalgrimaud annotation refactored to @IntegrationTest
. requested review
Rename <AppName>IntegrationTest annotation to IntegrationTest to avoid long length of annotation in case of long application names Fix jhipster#12460
<%_ if (cacheProvider === 'redis') { _%> | ||
import <%= packageName %>.RedisTestContainerExtension; | ||
<%_ } _%> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -18,19 +18,11 @@ | |||
-%> | |||
package <%= packageName %>.security; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -18,32 +18,20 @@ | |||
-%> | |||
package <%= packageName %>.service; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -39,7 +30,7 @@ import <%= packageName %>.service.UserService; | |||
import org.junit.jupiter.api.BeforeEach; | |||
<%_ } _%> | |||
import org.junit.jupiter.api.Test; | |||
<%_ if (databaseType === 'neo4j' || cacheProvider === 'redis' || (reactive && ['mysql', 'postgresql', 'mssql'].includes(prodDatabaseType))) { _%> | |||
<%_ if (reactive && ['mysql', 'postgresql', 'mssql'].includes(prodDatabaseType)) { _%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be removed too, this condition is the same as 'reactiveSqlTestContainers'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, its an unused import.
<%_ if (reactive && searchEngine === 'elasticsearch') { _%> | ||
import <%= packageName %>.repository.search.UserSearchRepository; | ||
<%_ } _%> | ||
import <%= packageName %>.security.AuthoritiesConstants; | ||
import org.junit.jupiter.api.Test; | ||
<%_ if (databaseType === 'neo4j' || cacheProvider === 'redis' || (reactive && ['mysql', 'postgresql', 'mssql'].includes(prodDatabaseType))) { _%> | ||
<%_ if (reactive && ['mysql', 'postgresql', 'mssql'].includes(prodDatabaseType)) { _%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be removed too, this condition is the same as 'reactiveSqlTestContainers'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, its an unused import.
import <%= packageName %>.config.TestSecurityConfiguration; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
<%_ if (databaseType === 'neo4j' || cacheProvider === 'redis' || (reactive && ['mysql', 'postgresql', 'mssql'].includes(prodDatabaseType))) { _%> | ||
<%_ if (reactive && ['mysql', 'postgresql', 'mssql'].includes(prodDatabaseType)) { _%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be removed too, this condition is the same as 'reactiveSqlTestContainers'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, its an unused import.
Bounty claimed at https://opencollective.com/generator-jhipster/expenses/30044 |
@swarajsaaj : approved, thanks for your work |
This composite annotation is meant to abstract complexity of common
Integration test annotations in a single file. This commit also adds the
annotation to required integration test files.
Fix #12460
Please make sure the below checklist is followed for Pull Requests.
When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding
skip-ci
label, you can still see CI build result at your branch.