-
Notifications
You must be signed in to change notification settings - Fork 128
Migrate org.eclipse.e4.core.tests to JUnit 5 (javax + jakarta variant) #1858
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.
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
Conversation
...ests/src/org/eclipse/e4/core/internal/tests/contexts/performance/ContextPerformanceTest.java
Show resolved
Hide resolved
...ests/src/org/eclipse/e4/core/internal/tests/contexts/performance/ContextPerformanceTest.java
Show resolved
Hide resolved
Test Results 1 764 files + 6 1 764 suites +6 1h 29m 12s ⏱️ + 3m 35s Results for commit 3aaa3cf. ± Comparison against base commit 02ffafb. This pull request removes 210 and adds 418 tests. Note that renamed tests count towards both.
This pull request removes 1 skipped test and adds 2 skipped tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Both test suites have 418 test in total, but the test suites now have different names. Partially because of Junit 5 and partially because I gave the javax variant a dedicated name, to avoid any further confusion. |
Everything looks good now. Unless there are any objections, I plan on merging later today. |
Sounds great!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this.
In general this looks good to me, I just have two minor remarks.
....e4.core.javax.tests/src/org/eclipse/e4/core/internal/tests/contexts/EclipseContextTest.java
Outdated
Show resolved
Hide resolved
...ests/src/org/eclipse/e4/core/internal/tests/contexts/performance/ContextPerformanceTest.java
Outdated
Show resolved
Hide resolved
- Add ContextPerformanceTest to test suite - Remove location markers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates.
The changes look good to me, although I have not reviewed each and every detail, but since all tests pass and the number of tests has even increased (in total), I think this is fine.
LGTM Thanks @ptziegler |
No description provided.