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.
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
Initial solution of Integration tests control. #185
Initial solution of Integration tests control. #185
Changes from 48 commits
e1ad209
8b61557
15b9fcd
79512f9
6ee894e
eb35565
107ba53
46ff776
297057f
5c62f86
464118b
559c24a
d0174fa
18c155f
d3bb3bb
3bb9810
4b91afe
d869448
cb82439
3be6f06
fd6e50f
f075b04
3f29f23
e2992b5
bc9c3ae
69c618f
3c22636
6d300a0
6170ac1
c4e0206
57b8dbe
08171ba
a5b5b61
b838cfc
3a72ba4
3f3ff49
715e9d1
5fd43e7
1e6e642
bfc194d
27820ec
d0b5266
e54443a
1db0673
acc44df
939d266
d36e6af
ed76338
b962730
b177140
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Btw, is this unit test coverage or combined? What does
sbt jacoco
do?If it runs
sbt test
under the hood, which excludes the ITs (I assume this is what's happening, judging by the output ofsbt jacoco
on my localhost), then it might make sense to update the naming/titles/sentences in the jacoco YAML files - so that things are a bit more explicit & bot comments generated in PRs would reflect that.Also, please rename this file to something like
jacoco_check_agent.yml
(we have_server
already so it would be more obvious)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.
Here is the overview:
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.
Yes but the naming of the files (potentially also steps) still deserves improving, such as this
jacoco_check.yml
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.
Sure, I am open to proposals.
Current naming is trying to be self-descriptive.
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.
so
jacoco_check.yml
->jacoco_check_agent.yml
/jacoco_check_agent_model.yml
❓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.
It could be done, yes. Byt why?
I would suggest to do future review here after project fixes issue with multiple java version need.
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.
Okay, let's address this outside of this PR.
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.
Problem is, that tasks that include testing like
assembly
don't take aliases into account and run them according to original definition. This creates an IMHO inconsistent behavior.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.
This problem can be solved by adding this line into module settings in build.sbt.
(assembly / test) := {},
Is that acceptable for modules with Integration tests and external dependencies?
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.
See the commit where with the proposed solution - ed76338