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.

Sign up for GitHub

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

Jump to bottom

Fix: reinstate tests #2368

Merged
merged 5 commits into from
Jun 11, 2024
Merged

Fix: reinstate tests #2368

merged 5 commits into from
Jun 11, 2024

Conversation

pieterlukasse
Copy link
Contributor

@pieterlukasse pieterlukasse commented May 2, 2024 •
edited
Loading

Reinstate tests, rolling back what was done in #1903

As far as I can see, there are just 4 same tests (out of 220) that always fail:

[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    CohortCharacterizationServiceTest.testExportGeneration:105->doTestExportGeneration:132->checkRequest:151 Checking dataitem org.ohdsi.webapi.cohortcharacterization.CohortCharacterizationServiceTest$ParamItem@130c39d9[analysisIds=[107],cohortIds=[2],isSummary=false,isComparative=false] expected:<1> but was:<0>
Error:    CohortCharacterizationServiceTest.testExportGenerationWithStrata:110->doTestExportGeneration:132->checkRequest:151 Checking dataitem org.ohdsi.webapi.cohortcharacterization.CohortCharacterizationServiceTest$ParamItem@747b18e5[analysisIds=[107],cohortIds=[2],isSummary=false,isComparative=false] expected:<1> but was:<0>
Error:    PermissionTest.permsTest:85->AbstractDatabaseTest.loadPrepData:112 Exception processing table name='public.sec_role'
Error:    PermissionTest.wildcardTest:101->AbstractDatabaseTest.loadPrepData:112 Exception processing table name='public.sec_role'
[INFO] 
Error:  Tests run: 220, Failures: 4, Errors: 0, Skipped: 0

I would advocate for only skipping these 4 for now instead of skipping all 220.

@chrisknoll
Copy link
Collaborator

Hmm, the last 2 about permission and wildcard tests were things I recently added which leverages the embedded db_unit test framework...i'm wondering why it doesn't work across a full test but I'm sure it ran on the individual ones (even both those together as I ran the entire test class).

I can pull down this branch and try to run the entire suite in debug and see if there's something that is a side-effect of the other tests that interfere with running this one test

@pieterlukasse
Copy link
Contributor Author

Thanks @chrisknoll . I'm looking at the CohortCharacterizationServiceTest ones (the fist two). So perhaps we can fix all 4 together 👍

@pieterlukasse pieterlukasse force-pushed the fix/reinstate_tests branch from 8b49e90 to 707b173 Compare May 3, 2024 19:56
@pieterlukasse
tmp: skip broken CohortCharacterizationServiceTest tests
5ea83a4 
This allows the other tests to be reinstated while these two
can then be fixed separately.
@pieterlukasse
Copy link
Contributor Author

@chrisknoll the tests you mentioned seem to fail only at the DELETE step:

loadPrepData(testDataSetsPaths, DatabaseOperation.DELETE);

@chrisknoll
Copy link
Collaborator

@chrisknoll the tests you mentioned seem to fail only at the DELETE step:

loadPrepData(testDataSetsPaths, DatabaseOperation.DELETE);

Thanks, I'm on a bit of a crunch and haven't' been able to dig in. But the DatabaseOperation.DELETE is where it's suppsoed to take the test data that is in the prep data set and delete any row that matches the data in there (effectively removing the rows that were inserted at the start of the test). I'll have to run through it by hand to trace through what part of it is failing the delete...is it a FK delete order problem? not sure yet....but I'm sure I can figure it out.

@pieterlukasse
Copy link
Contributor Author

is it a FK delete order problem?

It seems to fail at public.sec_role:

Exception processing table name='public.sec_role'

@pieterlukasse
Copy link
Contributor Author

@chrisknoll should I just disable the two PermissionTest ones as well for now and log a separate ticket so they can get fixed later?

@pieterlukasse
tmp: skip broken PermissionTest tests
ec6e298 
This allows the other tests to be reinstated while these two
can then be fixed separately.
@pieterlukasse pieterlukasse mentioned this pull request May 30, 2024
Closed
@pieterlukasse
tmp: skip broken StudyInfoTest
ccc748b 
...this one seems to only work depending on the order of execution...
it lacks the setup() method where a pre-filled db is guaranteed, like
for example in CohortCharacterizationServiceTest
@pieterlukasse
Copy link
Contributor Author

pieterlukasse commented May 30, 2024 •
edited
Loading

StudyInfoTest also started failing (side effect?), so I disabled that as well:

Error:  Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.101 s <<< FAILURE! - in org.ohdsi.webapi.test.feasibility.StudyInfoTest
Error:  testStudyCRUD(org.ohdsi.webapi.test.feasibility.StudyInfoTest)  Time elapsed: 0.068 s  <<< ERROR!
javax.persistence.EntityNotFoundException: Unable to find org.ohdsi.webapi.source.Source with id 1
	at org.ohdsi.webapi.test.feasibility.StudyInfoTest.testStudyCRUD(StudyInfoTest.java:63)

Summary: 5 unit tests have been disabled. The rest seems to be passing now ✅ :

[INFO] Results:
[INFO] 
Warning:  Tests run: 220, Failures: 0, Errors: 0, Skipped: 5

⚠️ Integration Tests (IT) seem to still be failing though 🤔 :

[INFO] Results:
[INFO] 
Error:  Errors: 
Error:    SecurityIT.testServiceSecurity » AmbiguousTableName COHORT
[INFO] 
Error:  Tests run: 6, Failures: 0, Errors: 1, Skipped: 1

The SecurityIT tests also reports a lot of errors like

2024-05-30T12:22:48.9955376Z 2024-05-30 12:22:48.970 INFO main org.ohdsi.webapi.test.SecurityIT - [] - failed service PUT:http://localhost:34829/WebAPI/reusable/0/version/0
2024-05-30T12:22:48.9957527Z 2024-05-30 12:22:48.971 INFO main org.ohdsi.webapi.test.SecurityIT - [] - testing service DELETE:http://localhost:34829/WebAPI/reusable/0/version/0
2024-05-30T12:22:48.9959824Z 2024-05-30 12:22:48.993 ERROR http-nio-auto-1-exec-4 org.ohdsi.webapi.util.GenericExceptionMapper - [] - javax.ws.rs.NotFoundException: There is no reusable version with id = 0.

or

2024-05-30T12:22:51.5852794Z 2024-05-30 12:22:51.580 INFO main org.ohdsi.webapi.test.SecurityIT - [] - tested service PUT:http://localhost:34829/WebAPI/conceptset/0 with code 500
2024-05-30T12:22:51.5853793Z 2024-05-30 12:22:51.580 INFO main org.ohdsi.webapi.test.SecurityIT - [] - failed service PUT:http://localhost:34829/WebAPI/conceptset/0
2024-05-30T12:22:51.5857909Z 2024-05-30 12:22:51.580 INFO main org.ohdsi.webapi.test.SecurityIT - [] - testing service PUT:http://localhost:34829/WebAPI/estimation/0
2024-05-30T12:22:51.5928720Z 2024-05-30 12:22:51.591 ERROR http-nio-auto-1-exec-7 org.ohdsi.webapi.util.GenericExceptionMapper - [] - java.lang.NullPointerException

Somehow the IT tests step finished with SUCCESS, so the errors are not being detected correctly. To be solved in a separate ticket, together with the disabled tests above perhaps.

@pieterlukasse
Copy link
Contributor Author

@chrisknoll here are the follow-up tickets:

@anthonysena anthonysena merged commit 7898f7e into OHDSI:master Jun 11, 2024
2 checks passed
@anthonysena anthonysena linked an issue Aug 6, 2024 that may be closed by this pull request
Enable GitHub Actions for unit tests #2243
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anthonysena anthonysena anthonysena approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable GitHub Actions for unit tests
3 participants
@pieterlukasse @chrisknoll @anthonysena