Feature/118 jacoco it test #123
Conversation
- This is not expected final solution. But is is example how to use Custom test configuration as Task.
- Introduces new solution for split of unit tests from Integration tests.
- Update of yml file for build and IntegrationTest.
- Extended build.yml to support Integration tests. - Replace "test" to Test in Dependencies. - Deleted extra yml file for Integration Tests.
miroslavpojer
requested review from
benedeki,
dk1844,
jakipatryk,
lsulak and
Zejnilovic
as code owners
There was a problem hiding this comment.
I finished the review for now. Please don't merge it yet, as this PR has similar "issues" (or rather, things to potentially change) like in AbsaOSS/atum-service#185.
.github/workflows/build.yml
Outdated
| - name: Add coverage to PR | ||
| id: jacoco | ||
| uses: madrapps/jacoco-report@v1.5 | ||
| uses: madrapps/jacoco-report@v1.6.1 |
There was a problem hiding this comment.
if you want to be consistent with AbsaOSS/atum-service#185, consider to extract this version elsewhere
There was a problem hiding this comment.
The version is used once only. The rest of the file do not follow this. In the atum-service project, this change produces an error.
.github/workflows/build.yml
Outdated
| - scala: 2.13.12 | ||
| scalaShort: "2.13" | ||
| overall: 0.0 | ||
| overall: 75.0 |
There was a problem hiding this comment.
I see what you did there :-D
ha, I didn't know that it's already this high :) cool, why not 👍
There was a problem hiding this comment.
As it's used only once, I think it's OK to be hard-coded.
There was a problem hiding this comment.
This can be done by team decision.
lsulak
dismissed stale reviews from salamonpavel and Zejnilovic
The base branch was changed.
JaCoCo core code coverage report - scala 2.13.12
|
JaCoCo slick code coverage report - scala 2.13.12
|
JaCoCo doobie code coverage report - scala 2.13.12
|
| # Aliases in this file expected usage of test file naming conventions: | ||
| # - "UnitTest" suffix for test files and Suites which define unit tests | ||
| # - "IntegrationTest" suffix for test files and Suites which define integration tests | ||
|
|
There was a problem hiding this comment.
What about a testAll alias?
There was a problem hiding this comment.
It can be done in project customization action if needed.
From a future point of view, testAll is not a good option.
There was a problem hiding this comment.
OK, why you don't see it as a good option?
There was a problem hiding this comment.
In the future, I hope we can support testPerformance, testStability, and similar test types.
So, as I said in the previous answer, test can be part of customization but not a generic solution proposal.
There was a problem hiding this comment.
Wait do you see these test to be actually part of the code and not "outside setup"? 🤔 (Actually not sure what testStability means.
And if they are created, what prevents us from (not) include them?
There was a problem hiding this comment.
I do want to fight about it. Customization space is open.
I will add testAll for this project, ok?
.sbtrc
Outdated
|
|
||
| # CPS QA types aliases | ||
| # * Unit tests | ||
| alias test=; testOnly *UnitTest |
There was a problem hiding this comment.
Idea:
What about making this plural. Sounds more natural to my non-native ear:
| alias test=; testOnly *UnitTest | |
| alias test=; testOnly *UnitTests |
NB! It affects all the renames bellow.
.sbtrc
Outdated
| alias test=; testOnly *UnitTest | ||
|
|
||
| # * Integration tests | ||
| alias testIT=; testOnly *IntegrationTest |
There was a problem hiding this comment.
Idea:
To be consistent, also plural:
| alias testIT=; testOnly *IntegrationTest | |
| alias testIT=; testOnly *IntegrationTests |
Prepared solution for Integration Tests and Jacoco code coverage measuring.