Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove hard-coded test finder from JUnitCore #953

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Remove hard-coded test finder from JUnitCore #953

merged 2 commits into from
Dec 11, 2023

Conversation

fedejeanne
Copy link
Contributor

@fedejeanne fedejeanne commented Nov 30, 2023 •
edited
Loading

What it does

Use the proper test finder according to what the container has. Before this PR, the JUnit4 finder was hardcoded and some tests were not being found.

Fixes #952

How to test

  1. Create a new Java project
  2. Create a class with nested tests and add JUnit5 to the classpath
  3. Right click on the project > Run As > JUnit Test
  4. Export the Ant Buildfiles of the project via File > Export > General > Ant Buildfiles
  5. The resulting build.xml file doesn't contain the Launch Configuration created in step 3.

After applying this PR, the launch configuration is exported.

Here's a screenshot of the difference in the build.xml files created before this PR (left) and after this PR (right)

image

Here are the generated files: build_files.zip

Author checklist

@iloveeclipse iloveeclipse force-pushed the remove_hard_coded_test_finder branch from 039a05a to 260b1e9 Compare November 30, 2023 20:05
@iloveeclipse
Copy link
Member

I wonder if you could craft a unit test for unit test finder :)

@fedejeanne
Copy link
Contributor Author

I actually discovered this bug while doing that :-)

I'm looking at JUnit3TestFinderTest and JUnit4TestFinderTest and trying to come up with a new JUnit5TestFinderTest. It will take a bit to get it done since I need to first try and cover all the possible tests JUnit5 can find so it's probably not going to be be ready for this PR.

@fedejeanne
Copy link
Contributor Author

@mkeller could you please take a look too? I noticed you coded this a couple of years back so you probably still remember some use cases and possible side effects?

@vogella
Copy link
Contributor

vogella commented Dec 1, 2023

@mkeller could you please take a look too? I noticed you coded this a couple of years back so you probably still remember some use cases and possible side effects?

@mkeller is inactive since a longer time.

@fedejeanne fedejeanne force-pushed the remove_hard_coded_test_finder branch from 260b1e9 to 36680c6 Compare December 4, 2023 13:35
@fedejeanne fedejeanne marked this pull request as draft December 4, 2023 13:36
@fedejeanne fedejeanne force-pushed the remove_hard_coded_test_finder branch 2 times, most recently from 8ba01a7 to 23b5b5c Compare December 4, 2023 13:47
@fedejeanne fedejeanne marked this pull request as ready for review December 4, 2023 13:48
@fedejeanne
Copy link
Contributor Author

@iloveeclipse I added some tests, they have been provided in #968 though since they need to exist regardless of this PR.
This makes #968 a pre-condition for this PR (that's also why there are 2 commits in this PR).

@fedejeanne fedejeanne force-pushed the remove_hard_coded_test_finder branch 2 times, most recently from 189f4ea to a5eddb3 Compare December 5, 2023 10:56
@fedejeanne
Copy link
Contributor Author

fedejeanne commented Dec 5, 2023 •
edited
Loading

I renamed JUnit4TestFinderTest to JUnitTestFinderTest and let it run as a parameterized test with 2 configurations: 1 for JUnit4 and 1 for JUnit5. This adds even more missing tests to test the functionality of the JUnit5TestFinder.

@fedejeanne fedejeanne force-pushed the remove_hard_coded_test_finder branch from a5eddb3 to 7a3f14f Compare December 5, 2023 13:28
@iloveeclipse
Copy link
Member

and some tests were not being found.

Many thanks for the added tests, but if I run them without the change on JUnitCore from this PR, they all pass.
So the where are tests for the actual problem?

@fedejeanne fedejeanne force-pushed the remove_hard_coded_test_finder branch from 7a3f14f to b83c53d Compare December 6, 2023 12:13
@iloveeclipse
Copy link
Member

So the where are tests for the actual problem?

Any hint here?

@fedejeanne
Copy link
Contributor Author

So the where are tests for the actual problem?

Any hint here?

I was looking into it when you wrote :-)

The current tests only provide the JUnit5TestFinder with the same test coverage that JUnit4TestFinder already had so they were only a "long due contribution".

I'll provide a regression test today

@fedejeanne fedejeanne force-pushed the remove_hard_coded_test_finder branch from b83c53d to 6750a02 Compare December 6, 2023 12:53
@fedejeanne
Copy link
Contributor Author

I provided a regression test: org.eclipse.jdt.junit.tests.JUnitTestFinderTest.testInnerClassWithNestedAnnotationIsFound()

@fedejeanne fedejeanne mentioned this pull request Dec 6, 2023
Closed
3 tasks
@fedejeanne fedejeanne force-pushed the remove_hard_coded_test_finder branch from 6750a02 to 4823400 Compare December 8, 2023 06:33
@iloveeclipse
Copy link
Member

I provided a regression test: org.eclipse.jdt.junit.tests.JUnitTestFinderTest.testInnerClassWithNestedAnnotationIsFound()

Perfect, thanks.
I will rebase now & if the tests will be green, merge.

@fedejeanne @iloveeclipse
Add test class JUnit5TestFinderJupiterTest
a357d18 
The class contains some tests that cover the current capabilities of
JUnit5TestFinder and it was missing from the tests.
This class does NOT resemble the other 2 existing classes:
JUnit3TestFinder and JUnit4TestFinder
@fedejeanne @iloveeclipse
Remove hard-coded test finder from JUnitCore
d9d0159 
The test finder should be obtained by looking into the container and
determining what kind of tests can be run there.
Rename JUnit4TestFinderTest to JUnitTestFinderTest and make it a
parameterized test that runs both in JUnit4 and JUnit5 mode and add a
regression test to it ("testInnerClassWithNestedAnnotationIsFound")

Fixes eclipse-jdt#952
@iloveeclipse iloveeclipse force-pushed the remove_hard_coded_test_finder branch from 4823400 to d9d0159 Compare December 11, 2023 14:46
@iloveeclipse iloveeclipse merged commit 54ab848 into eclipse-jdt:master Dec 11, 2023
7 checks passed
@iloveeclipse
Copy link
Member

@fedejeanne : many thanks, especially for tests.

@fedejeanne
Copy link
Contributor Author

@fedejeanne : many thanks, especially for tests.

My pleasure, thanks for reviewing!

@fedejeanne fedejeanne deleted the remove_hard_coded_test_finder branch December 11, 2023 19:43
@iloveeclipse iloveeclipse mentioned this pull request Dec 12, 2023
Closed
@fedejeanne fedejeanne mentioned this pull request Dec 23, 2023
Merged
@fedejeanne fedejeanne mentioned this pull request Apr 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iloveeclipse iloveeclipse iloveeclipse left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[JUnit5] Export "Ant Buildfiles": some Test Launch Configurations are not added to the resulting build.xml
3 participants
@fedejeanne @iloveeclipse @vogella