Convert tests to JUnit5 #2593
Convert tests to JUnit5 #2593
Conversation
Test Results 118 files ±0 118 suites ±0 13m 19s ⏱️ + 2m 31s Results for commit eb57c00. ± Comparison against base commit 12ac449. This pull request removes 706 and adds 706 tests. Note that renamed tests count towards both.This pull request removes 4 skipped tests and adds 4 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
In addition to the top comment, there are a few reasons I would like to get this integrated if there are no objections.
|
|
TL;DR The overall test counts look correct to me, this PR has expected number tests on Jenkins runs compared to latest master The discovered test count has gone up a lot because of how I migrated the parameterized browser test. This means all the IE tests are marked as skipped on non-Windows platform instead of simply not existing in the test report. This increases the test count by 194 (The number of browser tests), which can be seen as the difference in total tests from latest master and this PR. On those same links you can see the total number of tests that pass is the same (4196). I had originally identified a coupe of tests that were not running when I did the migration. See new |
|
Thanks Jonah! I have been converting tests one by one for quite some time with this being the end goal - thanks for bringing it to closure. |
...t.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java.orig
Outdated
Show resolved
Hide resolved
|
I am not thrilled by the new classes that used to be parametrized tests but I don't have better idea for now. Other than the *.orig file included by mistake it looks good to me. |
Because most all the tests remaining tests are in huge class hierarchy under Test_org_eclipse_swt_widgets_Widget they all need changing at once. The tests that are not under Test_org_eclipse_swt_widgets_Widget are generally pretty small, so included here to fully remove JUnit4 dependency. This means that junit-vintage-engine and org.junit removed from Require-Bundle of the manifest. Messages have been removed from assertions that didn't add value. The order of parameters to assertEquals and others changed, and for most of the calls I removed the message based on previous guidance. I tried to leave in messages that were more useful for documenting or diagnosing errors. But I removed messages that more or less repeated as a string what the item being tested is. A lot of things work quite differently in JUnit5. Here are some highlights that have affected or benefited this change: There is a better way of integrating screenshots into failed tests as tests can be intercepted between the test completion and the `@AfterEach` methods. Therefore we can take a screenshot prior to disposal, and then let the tear down message cleanup in a less convoluted way. See Test_org_eclipse_swt_widgets_Widget.afterTestExecutionCallback and tearDown methods to see this in action. The way to get a test name is quite different. Instead of a TestName `@Rule` a `TestInfo` can be obtained. The name returned is not exactly the same format, but where the name needed a specific format, I adjusted the code appropriately. See Test_org_eclipse_swt_browser_Browser.testInfo and Test_org_eclipse_swt_widgets_Widget.testInfo for a couple of use cases. Whole class parameterized tests don't really exist in a simple way in JUnit5. As there was very little use of this, rather than having complicated setup, I used sub-classes to provide the parameterization. See for example Test_org_eclipse_swt_widgets_DateTime and its new subclasses.
Thanks for leading the way on that (and looking through the history on doing the 3 -> 4 migration too!). I was only wanting to do StyledText, but things ballooned because of the huge hierarchy of tests. |
|
Is this ok to merge now? |
|
Yes it i. Merging. |
Because most all the tests remaining tests are in huge class hierarchy under Test_org_eclipse_swt_widgets_Widget they all need changing at once. The tests that are not under Test_org_eclipse_swt_widgets_Widget are generally pretty small, so included here to fully remove JUnit4 dependency.
This means that junit-vintage-engine and org.junit removed from Require-Bundle of the manifest.
Messages have been removed from assertions that didn't add value. The order of parameters to assertEquals and others changed, and for most of the calls I removed the message based on previous guidance. I tried to leave in messages that were more useful for documenting or diagnosing errors. But I removed messages that more or less repeated as a string what the item being tested is.
A lot of things work quite differently in JUnit5. Here are some highlights that have affected or benefited this change:
There is a better way of integrating screenshots into failed tests as tests can be intercepted between the test completion and the
@AfterEachmethods. Therefore we can take a screenshot prior to disposal, and then let the tear down message cleanup in a less convoluted way. See Test_org_eclipse_swt_widgets_Widget.afterTestExecutionCallback and tearDown methods to see this in action.The way to get a test name is quite different. Instead of a TestName
@RuleaTestInfocan be obtained. The name returned is not exactly the same format, but where the name needed a specific format, I adjusted the code appropriately. See Test_org_eclipse_swt_browser_Browser.testInfo and Test_org_eclipse_swt_widgets_Widget.testInfo for a couple of use cases.Whole class parameterized tests don't really exist in a simple way in JUnit5. As there was very little use of this, rather than having complicated setup, I used sub-classes to provide the parameterization. See for example Test_org_eclipse_swt_widgets_DateTime and its new subclasses.