OpenMRS-core build failure

I am experiencing the same problem with AdministrationServiceTest. The tests which fail are:

getAllGlobalProperties_shouldReturnAllGlobalPropertiesInTheDatabase purgeGlobalProperty_shouldDeleteGlobalPropertyFromDatabase

The both fail with: AssertionError: expected:<21> but was:<25>

This happens every time I run the full set of tests using mvn clean test. However, if I run the full suite of tests from within IntelliJ, the problem does not occur.

I placed some print statements in the tests to list the names and values of all the global properties, and re-ran the tests from IntelliJ and the command line. There are four properties which appear in the latter but not the former:

test1.mandatory=false
test1.started=true
test2.mandatory=false
test2.started=true

I then did a grep on the codebase and found one file, ModuleFactoryTest.java which looked very guilty. Running the test suite from the command line, skipping this test, results in no tests failing: mvn test -Dtest=!ModuleFactoryTest*

Placing print statements in the @Before in ModuleFactoryTest shows that initially these properties are not present. After running tests within this class, these properties do appear - and don’t seem to remove themselves. Maybe this is correct behaviour, I’m not sure what the intention is.

I believe the order of execution of JUnit tests is unpredictable. So it seems likely that if ModuleFactoryTest creates extra global properties for new modules before AdministrationServiceTest runs, these will cause the latter to fail. (Also, the ModuleFactory creates new modules via the Daemon and DaemonThread classes, so separate threads are used and order of execution is unpredictable - this may be relevant).

One approach to fixing this might be to force ModuleFactoryTest to clean up after itself (assuming that is desirable). Another might be to make AdministrationServiceTest count how many global properties exist before it creates/removes any, and then count them again immediately afterwards, rather than expecting a fixed number (21) to be present initially. One could do both fixes.

Thanks @jerryb8 for this exhaustive investigation. Both fixes look fine to me. Would you like to create a ticket and raise a pull request?

ok, will do.

I have created a ticket, but was not sure where to put it, as it is test-only. It appears as CORE-5 rather than as TRUNK-* which wasn’t what I’d intended.

It would be great to include the ticket link here, for completeness. :slight_smile:

oops
 the ticket is:

https://issues.openmrs.org/browse/CORE-5

Thanks! :slight_smile:

I have made it ready for work such that you claim it and get started. Can you also edit the ticket to include a link for this talk thread? Hopefully you have already seen this: https://wiki.openmrs.org/display/docs/Pull+Request+Tips

1 Like

I put in a pull request, https://github.com/openmrs/openmrs-core/pull/2702 and this shows one failure https://coveralls.io/builds/18505882 due to decreased code coverage.

This is puzzling because the two changed tests make exactly the same calls as they did previously, and the classes which show decreased coverage don’t appear to relate to the work of these tests, at least not directly.

I’ve been trying to find an easy way to see which lines were, but are no longer, covered - but I cannot see how to do this.

Thanks for the feedback. Did you just forget to take the JIRA ticket through the process outlined here? https://wiki.openmrs.org/display/docs/Pull+Request+Tips

I didn’t get as far as “Request Code Review” - I thought that as my changes had caused a check fail, it wasn’t yet ready for that. Sorry if I have misunderstood the process, if there’s a way of getting it wrong, I will find it :slight_smile:

As it is now, any one can claim the ticket and work on it before you are even ready for review. Did you actually go through this? https://wiki.openmrs.org/display/docs/Pull+Request+Tips

Sorry, I don’t understand. I’m looking at https://issues.openmrs.org/browse/TRUNK-5417 and it shows “Assignee: Jerry Bate”, which I thought meant I’d claimed it.

I have now clicked on “Claim issue” which changes the status to “IN PROGRESS”, and leaves me as the Assignee. Maybe when details were transferred from CORE-5 to TRUNK-5417 my name was retained as assignee, but the status was “Ready for work”, as befits a new ticket.

Yes, I did read the documentation, but still managed to get it wrong!

I am now unsure whether I should be submitting this for code review when the code coverage is giving me a fail, albeit for reasons I don’t understand.

Jerry

After your having claimed it, the JIRA ticket now looks good. Yes go ahead to raise a pull request and take the ticket through the process outlined at https://wiki.openmrs.org/display/docs/Pull+Request+Tips

About the coverage, this looks like what you are looking for: https://talk.openmrs.org/t/test-coverage-reduced-after-adding-unit-tests/17034

ok, I have requested a code review. I have left fields blank where I wasn’t sure what to fill in.

Merged and ticket closed. Thank you so much @jerryb8 :smile: