Skip to content
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

Merged
merged 50 commits into from
May 23, 2024

Conversation

miroslavpojer
Copy link
Collaborator

@miroslavpojer miroslavpojer commented Apr 12, 2024

  • 'sbt test' now does no run Integration tests
  • To run IT ot DB tests is needed to call extra command defined in .sbtrc

Depends on #196

- 'sbt test' now does no run Integration tests
- To run IT ot DB tests is needed to call extra command defined in .sbtrc
@miroslavpojer miroslavpojer self-assigned this Apr 12, 2024
.github/workflows/jacoco_check.yml Show resolved Hide resolved
.github/workflows/jacoco_check.yml Outdated Show resolved Hide resolved
.sbtrc Outdated Show resolved Hide resolved
.sbtrc Outdated Show resolved Hide resolved
.github/workflows/jacoco_check.yml Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
README.md Outdated

### Test controls

The project uses the `.sbtrc` [(link)](https://www.scala-sbt.org/1.x/docs/Best-Practices.html#.sbtrc) file to manage and control the execution of integration tests. The Integration tests are the ones with the tag `Integration test.` Below are the commands configured in the `.sbtrc` file to facilitate different testing scenarios:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tag IntegrationTest

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit - cb82439.

README.md Outdated
```
alias test=; testOnly -- -l IntegrationTest

# `-l IntegrationTest`: This switch excludes all tests labeled as `IntegrationTest,` allowing only only executing unit tests.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# `-l IntegrationTest`: This switch excludes all tests labeled as `IntegrationTest,` allowing only only executing unit tests.
# `-l IntegrationTest`: This switch excludes all tests labeled as `IntegrationTest,` allowing only executing unit tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit - 3be6f06.

@@ -50,7 +50,7 @@ jobs:
- name: Add coverage to PR
Copy link
Collaborator

@lsulak lsulak Apr 22, 2024

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 of sbt 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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the overview:

  • agent - unit test only (no IT/DB test in module)
  • model - unit test only (no IT/DB test in module)
  • database - db test only (no IT/Unit test in module)
  • server - integration test only (no Unit/DB test in module)

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finished the review for now. The biggest thing I'd like to be changed definitely is the scope of these tags and then few improvements/answers regarding the YML jacoco file(s).

Copy link

github-actions bot commented May 14, 2024

JaCoCo model module code coverage report - scala 2.13.11

Build Failed

Copy link

github-actions bot commented May 14, 2024

JaCoCo server module code coverage report - scala 2.13.11

Overall Project 70.78%

There is no coverage information present for the Files changed

Zejnilovic
Zejnilovic previously approved these changes May 14, 2024
lsulak
lsulak previously approved these changes May 15, 2024
lsulak
lsulak previously approved these changes May 16, 2024
@benedeki benedeki added the dependent The item depends on some other open item (Issue or PR) label May 16, 2024

# CPS QA types aliases
# * Unit tests
alias test=; testOnly *UnitTests
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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

- Added examples how to manage tests in rest of sbt commands (assembly, publish).
build.sbt Outdated
jacocoExcludes := jacocoProjectExcludes()
jacocoExcludes := jacocoProjectExcludes(),
(assembly / test) := {},
(publish / test) := { (Test / testOnly).toTask(" *UnitTests").value }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why a space here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it could be removed.

lsulak
lsulak previously approved these changes May 21, 2024
@benedeki benedeki removed the dependent The item depends on some other open item (Issue or PR) label May 22, 2024
@benedeki
Copy link
Contributor

This item depends on:

@miroslavpojer miroslavpojer merged commit cc8892e into master May 23, 2024
7 of 10 checks passed
@miroslavpojer miroslavpojer deleted the feature/new-sbt-integration-tests branch May 23, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants