Following the discussion in this thread, hereafter the piece of interest
We used that group when we didn’t have access to saucelabs and we
couldn’t afford to run all tests, because the build was getting way too
slow. Now, we should probably get rid of it since we can run all tests
in acceptable time.
So to fix things…
Remove the group from the CI profile config so that all tests are run.
Mark failing tests with @Ignore.
Get rid of the @Category(BuildTests.class) annotation.
Remove annotating test with @Category(BuildTests.class) from the UI test guidelines
As you can see, they are not complicated tasks but all in all they
are thorny because we run the risk to break the test suite execution
(e.g., running test cases that shouldn’t run).
My proposal follows:
Find all the failing test cases to annotate with @Ignore.
The analysis shall include both test with BuildTest.class annotation
(they should be fine) and without the annotation (they are likely to
fail). At the end of this activity we get a list of all tests to mark as
Ignored. I’m already working on that.
Open issue for the ignored tests. Here we have 2 options, a) an issue for each test or b) a
“cumulative” issue which include all the tests. In case a) we can
address the issue one by one in the future, although it requires a bit
more effort since we have to open an issue for each test and commit the
ignore test one by one (You will cope with some pull requests).
Conversely for b) just one issue and one commit are sufficient, but it
would be troublesome in the future to track the fixed test cases, we
should split the issue as long as the tests are fixed.
I don’t know exactly how many tests to ignore, from a first analysis I counted dozen tests
Strictly and only after the former task, open only one issue to get rid
of the group from the CI profile config and of Build.test annotation
from all tests. Then, commit all the tests and the pom together.
I don’t think we should have a separated commit for the pom and another for the test.
Then I’ll update the wiki page.
Let me know what you think of.
Thank you in advance.
Regarding task 1:
I have executed the tests have BuildTest.class annotation and the ones missing it. All in all junit execute 123 tests among them, there are:
6 test cases without BuildTest.class which pass, namely:
AddDeleteServiceTest.java
AdmitExitInpatientTest.java
ComeBackToApplicationAfterLogoutTest.java
DeleteAppointmentBlockTest.java
EditDemographicTest.java
EditServiceTest.java
- 8 tests without BuilTest.class which fail, namely:
AddRolesToUserTest
AddUserTest.java
LocationSensitiveApplicationTestBase
ManageProviderSchedulesTest.java
ReferenceApplicationTestBase
RecordMergeIssueTest.java
RegisterUnidentifiedPatientTest.java
TransferToWardServiceTest.java
To note ReferenceApplicationTestBase and ManageProviderSchedules that shouldn’t be executed because they have no runnable method. They are meant to be superclass.
6 test cases without BuildTest.class which are ignored, namely:
AddFormTest.java
DeleteFormTest.java
EditFormTest.java
EyeReportTest.java
UnidentifiedPatientKeyboardTest.java
UsingBackButtonInMergePatientTest.java
The other test have the annotation BuildTest.class and pass (as we expect, they are the tests currently executed for CI).
Regarding the pom.xml, how would like to work?
Let’s say we just have one commit including all the test cases and the pom.xml.
If the execution gets broken, with only one revert you can get back to the former situation.
Contrary, if we have a commit on its own for pom.xml, you have to revert it and the commit with the test cases, not hard to do, but maybe it is less suitable than one commit.
Let me know
I’ve been refactoring the code. I would like to focus your attention on an aspect.
In the test suite, we have some father test classes
without the BuilTest annotation, ReferenceApplicationTestBase above all.
Getting rid of the annotation BuilTest means that this class will be executed.
Its execution results in a fail, reasonably the “missing test method” exception arises.
How would you like to cope with it?
Proposals from my side:
1 We add an @Ignore(“This is a super class of all tests and it should be never executed”) annotation at class level. I would also make ReferenceApplicationTestBase an abstract class.
Of course, we say something on the usage of this annotation in the wiki.
2 We can create a new Category, for instance “ExcludeTest” and apply it to ReferenceApplicationTestBase (and other super class in the suite). In the pom.xml
a new tag, the excludeGroup, replace org.openmrs.reference.groups.BuildTests.
The aim is to reserve in somehow the use of @Ignore only for failing tests, while the “exclude” category, it is for tests that at design time we don’t want to be executed.
3) We keep on using BuildTest category
It is not a big deal from my side.
In the current situation, it is more convenient to specify which test to exclude rather than marking one by one the test to execute.
The best option would be to automatically exclude the test cases without @Test. I want to investigate further if it is possible.
Well coming to an end, if I can’t find a simple and automatic way to exclude test cases without @Test, we can keep on using @BuildTest.
@raff, what’ s your suggestion?
Thank you
Yes, this is another option that I forgot to propose.
Well, we will have a convention on the test name, I’ll update the wiki accordingly.
@dkayiwa, we will go further by removing the annotation in all the test case, post-pending the base suffix to test case name when necessary and fixing the pom.xml accordingly.
The folder openmrs-distro-referenceapplication/ui-tests/src/test/java/org/openmrs/reference/groups/ willbe deleted.
To this end I’ll do just only one commit, this is it mostly for our convenience, if something gets broken with only one revert we can get back to a running configuration.
Thank you,
D.
