refactor entity's integration test class #10686
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stonedMoose
commented
Oct 26, 2019
generators/entity-server/files.js
Outdated
| ] | ||
| }, | ||
| { | ||
| condition: generator => generator.dto === 'mapstruct' && (generator.databaseType === 'sql' || generator.databaseType === 'mongodb' || generator.databaseType === 'couchbase'), |
There was a problem hiding this comment.
I'm not sure about this condition:
The fromIdmethod of the mapper is generated only when DB are sql or mongodb or couchbase, but the test is only generated when the DB is sql...
stonedMoose
force-pushed
the
move-some-integration-test-to-appropriate-unit-test-classes
branch
4 times, most recently
from
23d21da to
676e15b
Compare
murdos
reviewed
Oct 26, 2019
generators/entity-server/files.js
Outdated
| templates: [ | ||
| { | ||
| file: 'package/domain/EntityTest.java', | ||
| renameTo: generator => `${generator.packageFolder}/domain/${generator.asEntity(generator.entityClass)}Test.java` |
There was a problem hiding this comment.
Suggested change
| renameTo: generator => `${generator.packageFolder}/domain/${generator.asEntity(generator.entityClass)}Test.java` | |
| renameTo: generator => `${generator.packageFolder}/domain/${generator.entityClass}Test.java` |
There was a problem hiding this comment.
Thank you for the remark, I took it in account in my last commit.
I fixed the missing import to UUID in MapperTest too :)
stonedMoose
force-pushed
the
move-some-integration-test-to-appropriate-unit-test-classes
branch
from
cc0f403 to
8b7764d
Compare
murdos
approved these changes
Oct 26, 2019
move tests to appropriate test classes.
stonedMoose
force-pushed
the
move-some-integration-test-to-appropriate-unit-test-classes
branch
from
8b7764d to
03abdc1
Compare
|
Thanks @stonedMoose and congrats for your first contribution! |
|
Thanks a lot ! |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I use Jhipster for a few months now, and I was surprised to find some tests in EntityResourceIT which does not belong here in my opinion:
The first one, is the
fromIdtest which appear when mapstruct is involved. This test is not in anyway an integration test and it does not test the Resource but the Mapper. So I propose to move it to a class named EntityMapperTestThe last two tests which do not belong to EntityResourceIT are the
equalsVerifier. These tests ensure that theequalsmethod on Entity or DTO works as intended, so they should be respectively inEntityTestandEntityDTOTest.It's not a big improvment and it will not change the face of the earth, but it make sense to me, I hope it will make sense to you too.
Plus it will lower the amount of integration test and increase the number of unit test in the generated apps.