test(openchallenges): improve organization service service unit test coverage

[mdsage1]

Description

Sonar unit organization service test coverage has been increased to 52%, from 19%, for service package by writing unit tests for a single class, OrganizationService.

Related Issue

#2389
#2063
#1528
#2348
#2388

Fixes #(issue)

Changelog

See files Changed section

• Add 2 unit tests for getOrganization and other methods

Preview

[mdsage1]

Sonar suggested I remove all public modifiers so I removed them.

[mdsage1]

PR is ready for review. 2 Unit tests have been added for OrganizationService. Line 68 of the code has a public modifier which Sonar suggests I remove but that causes a formatting issue. @tschaffter Let me know which error/issue tracker takes priority here. I've prioritized the springBoot built in formatting.

[mdsage1]

@tschaffter please review my PR

[tschaffter]

PR is ready for review. 2 Unit tests have been added for OrganizationService. Line 68 of the code has a public modifier which Sonar suggests I remove but that causes a formatting issue. @tschaffter Let me know which error/issue tracker takes priority here. I've prioritized the springBoot built in formatting.

Is this issue still relevant? It looks like you resolved the formating issue (with nx run openchallenges-organization-service:format?).

You are adding two unit tests to the organization service in this PR but the Sonar app does not report an increase in coverage. Do you know why? Do you see an increase in coverage using JaCoCo?

[mdsage1]

PR is ready for review. 2 Unit tests have been added for OrganizationService. Line 68 of the code has a public modifier which Sonar suggests I remove but that causes a formatting issue. @tschaffter Let me know which error/issue tracker takes priority here. I've prioritized the springBoot built in formatting.

Is this issue still relevant? It looks like you resolved the formating issue (with nx run openchallenges-organization-service:format?).

You are adding two unit tests to the organization service in this PR but the Sonar app does not report an increase in coverage. Do you know why? Do you see an increase in coverage using JaCoCo?

@tschaffter Yes I have documented the increase in coverage seen in Jacoco, from 19% to 52%, in the Issue #1528 which is what this PR is addressing. I left the Sonar only information that is auto generated in this PR since I thought it might be overkill to add both. I did resolve the formatting issue after making the comment about it.

[mdsage1]

PR is ready for review. 2 Unit tests have been added for OrganizationService. Line 68 of the code has a public modifier which Sonar suggests I remove but that causes a formatting issue. @tschaffter Let me know which error/issue tracker takes priority here. I've prioritized the springBoot built in formatting.

Is this issue still relevant? It looks like you resolved the formating issue (with nx run openchallenges-organization-service:format?).
You are adding two unit tests to the organization service in this PR but the Sonar app does not report an increase in coverage. Do you know why? Do you see an increase in coverage using JaCoCo?

@tschaffter Yes I have documented the increase in coverage seen in Jacoco, from 19% to 52%, in the Issue #1528 which is what this PR is addressing. I left the Sonar only information that is auto generated in this PR since I thought it might be overkill to add both. I did resolve the formatting issue after making the comment about it.

@tschaffter I've made the requested changes to the comment lines character counts.

[sonarcloud]

Quality Gate passed for 'schematic-api'

Issues
0 New issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.1% Duplication on New Code

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed for 'openchallenges-app'

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed for 'openchallenges-challenge-service'

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed for 'openchallenges-image-service'

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[mdsage1]

PR marked as draft after noticing that there were new changes upstream that needed to be synced to current repo branch. This triggered a number of test failures related to schematic_api/...
@tschaffter I'm unsure what has occurred here as none of this is connected to the unit tests I've implemented. Do you have any idea what's happened? It looks like PR #2532 has no evidence of CI/CD checks but has a dpull check listed. I can't decipher if the issue came from this PR but it does indicate there was a change to the latest CSV dump files. Here are the errors:

FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeDependencyArray::test_return_display_names - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeDependencyArray::test_return_ordered_by_schema - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeDependencyArray::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeDependencyPage::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeDependencyPage::test_return_ordered_by_schema - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeDependencyPage::test_return_display_names - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeIsRequired::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeValidationRules::test_success - AssertionError: assert ['detail', 'status', 'title'] == ['validation_rules']
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetConnectedNodePairPage::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetSchemaAttributes::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetConnectedNodePairArray::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetPropertyLabel::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetComponent::test_parameters - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetComponent::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_endpoints.py::TestGetNodeProperties::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetNodeValidationRuleArray::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetComponent::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetConnectedNodePairArray::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetNodeDependencyPage::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetNodeIsRequired::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetNodeProperties::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetSchemaAttributes::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetConnectedNodePairPage::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetNodeDependencyArray::test_success - assert 500 == 200
FAILED schematic_api/test/test_schema_controller_impl.py::TestGetPropertyLabel::test_success - assert 500 == 200
FAILED schematic_api/test/test_manifest_validation_controller_impl.py::TestValidateManifestJson::test_success_no_errors - assert 500 == 200
FAILED schematic_api/test/test_manifest_validation_controller_impl.py::TestValidateManifestJson::test_success_one_error - assert 500 == 200
FAILED schematic_api/test/test_manifest_validation_controller_impl.py::TestValidateManifestCsv::test_success_no_errors - assert 500 == 200
FAILED schematic_api/test/test_manifest_validation_controller_impl.py::TestValidateManifestCsv::test_success_with_one_error - assert 500 == 200
FAILED schematic_api/test/test_tangled_tree_impl.py::TestGetTangledTreeLayers::test_success - assert 500 == 200
FAILED schematic_api/test/test_tangled_tree_impl.py::TestGetTangledTreeText::test_success - assert 500 == 200
FAILED schematic_api/test/test_manifest_validation_endpoints.py::TestValidateManifestJson::test_success - AssertionError: 500 != 200 : Response body is : {
FAILED schematic_api/test/test_manifest_validation_endpoints.py::TestValidateManifestCsv::test_success - AssertionError: 500 != 200 : Response body is : {
=== 33 failed, 185 passed, 18 deselected, 4579 warnings in 74.29s (0:01:14) ====
Warning: run-commands command "poetry run pytest -m 'not secrets'" exited with non-zero status code

NX Running target test for 17 projects and 4 tasks they depend on failed

Failed tasks:

• schematic-api:test

Error: Process completed with exit code 1.

[tschaffter]

Hey @andrewelamb, Maria bumped into the above Schematic test issues after updating her feature branch with main. Do these test failures seem familiar?

[andrewelamb]

@tschaffter There was a change to schematic that intorduced soem breaking changes the the API in the monorepo, involving both claled methods and the data model used for testing. These were fixed by this PR.. So if this commit isn't in the branch the schematic API tests will fail.

[andrewelamb]

@tschaffter Just curious, why are the schematic API tests beign run for this PR?

[tschaffter]

@tschaffter Just curious, why are the schematic API tests beign run for this PR?

That's an excellent question. I will document here my findings here as I explore this issue.

Troubleshooting

Here are the logs of the CI workflow that run for the last commit pushed by Maria to this PR. One of the first job of the workflow shows the "Base SHA" and "Head SHA" that nx uses to identify the list of affected projects for which tasks like lint, build, test, etc. will be run by the CI workflow.

Base SHA
ff72e18

Head SHA
75645b6

• The Base SHA points to an old commit in main ⚠️
  • The latest commit as of now is 70f2f7b.
  • The commit the Base SHA is pointing to is ff72e18.
    • This is a commit pushed to main about two weeks ago.
    • This is likely the commit that was used to create this feature branch as this PR has been opened between these two commits to main: ff72e18 and 08c4c73.
• The Head SHA is the commit pushed to this PR. ✅

In comparison, here are the SHAs for the first commit pushed to this PR.

Base SHA
ff72e18bec3b1b3a64b0566aac914b9ddb07caa7

Head SHA
9927dcac5704ddee0841c6bcff5b6273536476f6

Update 2024-02-28

It is likely that @mdsage1 first pushed the last commit mentioned above BEFORE she updated the main branch of her forks based on a discussion on Slack.

Since then, Maria updated the main branch of her fork and this actually translated to an update of the Base SHA when she pushed a new commit to this PR.

Conclusion

In the current context, when opening a PR from a fork, the Base Sha should refers to 1) the last commit to the main branch that 2) passed successfully all the checks.

Point 1) is confirmed as we this in this PR that the Base SHA changed when Maria updated the main branch of her fork on GitHub.

Point 2) is derived from this Nx comment:

Conceptually, what we want is to use the absolute latest commit on the master branch as the HEAD, and the previous successful commit on the master as the BASE. Note, we want the previous successful one because it is still possible for commits on the master branch to fail for a variety of reasons.

[andrewelamb]

@tschaffter After further investigation in the current version of main, the schemati api tests are faling as well, this also has to do with the breaking changes made by the most recent version of schematic. This will be fixed by this PR.

[sonarcloud]

Quality Gate passed for 'openchallenges-organization-service'

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[tschaffter]

@tschaffter Just curious, why are the schematic API tests beign run for this PR?

@andrewelamb I can now answer your question (see details). In a nutshell, Nx identifies the affected projects using two SHA: the Head and Base SHAs. The Head SHA refers to the commit ID of the last commit pushed to a feature branch/PR. Determining the Base SHA is a bit more tricky:

• Expected: The Base SHA should refer to the last successful commit pushed to the main branch of the official repo (this repo).

• Actual: The Base SHA refers to the last successful commit pushed to the main branch of the fork that opened the PR. The key lesson here is the reference to the main branch of the fork, not of the official repo. Hence, it is important to keep your fork's main branch of to date before pushing new commits.

I will update the CI workflow so that the Base SHA refers to the main branch of the monorepo.

[mdsage1]

@tschaffter Can you review this pr please?