-
Notifications
You must be signed in to change notification settings - Fork 215
Convert tests to JUnit5 #1377
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
Convert tests to JUnit5 #1377
Conversation
|
There are more commits coming on this PR |
Test Results 584 files - 1 584 suites - 1 13m 47s ⏱️ -1s For more details on these failures and errors, see this check. Results for commit 863c40b. ± Comparison against base commit 6574af3. This pull request removes 1331 and adds 1334 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| } | ||
|
|
||
| private static void cxx20SetUp() { | ||
| @BeforeEach |
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.
at least for this test this change did not worked well:
- as far as I can see in my local run
IndexBindingResolutionTestBase.setUpStrategy()fires and runs parser beforeIndexConceptTests.cxx20SetUpis fired so it's already too late to tell parser to recognize concepts - could be that a few other tests are affected by similar issue
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 having a sneak peak - I think IndexConceptTest should do:
@Override
@BeforeEach
protected void setUpStrategy() throws Exception {
TestScannerProvider.sDefinedSymbols.put("__cpp_concepts", "201907L");
super.setUpStrategy();
}instead of its own BeforeEach.
I misunderstood something in my first pass in refactoring and I will correct it before marking ready for review.
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.
BTW The most recent push does not resolve this yet.
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.
The problem I introduced is that some setUp methods were like this:
setUp() {
super.setUp()
localsetup codehere
}
And some were like this:
setUp() {
localsetup codehere
super.setUp()
}
And I didn't adequately pay attention to the difference in these two.
This isn't needed as JDT automatically injects what it needs when running tests.
…nit5 SemanticTestBase is the base class for a huge hierarchy of tests and all those test updates are included in this commit.
b8ad90f to
863c40b
Compare
|
The sizes of the diff can make it hard to see the important stuff. I think there should be some combination of
This makes the number of diff manageable and easy enough to review. |
|
I have decided to split this up into separate PRs as reviewing multiple commits in a single PR made it hard to see what was happening reliably. I will label them all https://github.com/eclipse-cdt/cdt/issues?q=state%3Aopen%20label%3Ajunit5 to make the easier to follow. |

#1375 identified that some tests are not running in JUnit5 runner because discovery is different than JUnit3 runner.
This PR changes the tests to use JUnit5 (in core tests only for now). However much of the purpose of this PR is to modernize the tests. This PR also works around eclipse-jdt/eclipse.jdt.ui#2591
Fixes #1375