refactor(test): Refactor QueryValidator usage in tests #4091
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contribution guidelines, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.
|
@enchobelezirev can you please sign the ECA? |
enchobelezirev
force-pushed
the
refactor(test)/improve-queryvalidator-testing-strategy
branch
from
8fda76e
to
4bb5da0
Compare
|
Hello @paullatzelsperger, I have fixed it :) |
enchobelezirev
changed the title
enchobelezirev
changed the title
|
Hello, I have seen that there is already defined class for testing the Please, correct me if I am wrong or I do not understand something. I am new to the project and there is high chance that I misunderstood the issue. Thanks! |
There was a problem hiding this comment.
this is not about the project but to keep a good testing strategy, here we have 2 components tested together in the same integration test for no reason. Dependency injection is used to make classes being testable by their own apart from specific cases, otherwise we'll end up bloated by tons of (harder to maintain) integration tests.
The issue is pretty clear about the desired outcome: "avoid unnecessary integration tests". The validator should be unit tested, since it's dependent from the object type it's validating, there should be a "generic behavior" test (that as you said it already exist, and then some tests for every class that's used (behaviors that currently are verified in the service tests and just need to be moved out somewhere else)
|
@ndr-brt, I will see how to modify the changes so that it complies with the issue. |
enchobelezirev
force-pushed
the
refactor(test)/improve-queryvalidator-testing-strategy
branch
from
fa04af0
to
098cea1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #4091 +/- ##
==========================================
+ Coverage 71.74% 74.81% +3.06%
==========================================
Files 919 988 +69
Lines 18457 20261 +1804
Branches 1037 1139 +102
==========================================
+ Hits 13242 15158 +1916
+ Misses 4756 4607 -149
- Partials 459 496 +37 ☔ View full report in Codecov by Sentry. |
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.verify; | ||
|
|
||
| class AssetServiceImplIntegrationTest { |
There was a problem hiding this comment.
As reported in the issue and in my latest 2 comments, please avoid integration tests and transform them in unit tests on the validator.
|
This pull request is stale because it has been open for 7 days with no activity. |
|
This pull request was closed because it has been inactive for 7 days since being marked as stale. |
What this PR changes/adds
QueryValidatorin testsWhy it does that
control-plane-aggregate-servicesprojectFurther notes
*Packages org.eclipse.edc.connector.controlplane.services.contractagreement and their test packages
Linked Issue(s)
Closes #3731
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.