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.

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

Correct enumeration of tests when running tests from a folder (#920) #1066

Conversation

erik-brangs
Copy link
Contributor

@erik-brangs erik-brangs commented Jan 14, 2024

What it does

Currently, when running tests from a source folder with JUnit 5, all tests that share a package with those packages in the selected source folder will be run (see #920). This attempts to fix this by enumerating all tests as suggested in #920.

I don't know if JDT uses a mocking framework so I mocked the needed classes by hand.

How to test

I've added a test case. I'm not sure if that is sufficient.

Author checklist

Edit: I've added a few more tests so I've set the checkmark for through testing.


@Override
public IDebugTarget getDebugTarget() {
// TODO Auto-generated method stub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have not looked into the details. but please do remove all "TODO"s
also there alsoready is a MockLaunch
org.eclipse.jdt.debug.tests.connectors.MockLaunch, please choose another name. Also it would be good to somehow have less boiler plate code - maybe extend Launch like NoopLaunch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I've changed the code to use Launch which seems to work fine for the test cases.

@erik-brangs erik-brangs force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch 2 times, most recently from 979a719 to 6101e2e Compare January 31, 2024 20:23
jukzi
jukzi previously requested changes Feb 1, 2024
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test is not executed on jenkins. please reference it to be executed
https://ci.eclipse.org/jdt/job/eclipse.jdt.ui-github/job/PR-1066/3/consoleText

@erik-brangs erik-brangs force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch 2 times, most recently from 4ef5141 to 3e539ed Compare February 1, 2024 07:39
@erik-brangs
Copy link
Contributor Author

I've changed the test to JUnit 4 and added a suite.

@jukzi
Copy link
Contributor

jukzi commented Feb 1, 2024

jenkins test fail with

java.lang.AssertionError: The declared package "" does not match the expected package "p1". Actual: 2
	at org.eclipse.jdt.ui.tests.jarexport.FatJarExportTests.buildProject(FatJarExportTests.java:314)
	at org.eclipse.jdt.ui.tests.jarexport.FatJarExportTests.assertFatJarExport(FatJarExportTests.java:205)
	at org.eclipse.jdt.ui.tests.jarexport.FatJarExportTests.createAndRunFatJar(FatJarExportTests.java:175)
	at org.eclipse.jdt.ui.tests.jarexport.FatJarExportTests.exportExternalLib(FatJarExportTests.java:876)

please fix

@erik-brangs erik-brangs force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch from 3e539ed to 1bfd2ab Compare February 1, 2024 18:08
@erik-brangs erik-brangs requested a review from jukzi February 1, 2024 18:32
@jukzi jukzi force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch from 1bfd2ab to 49c0b80 Compare February 2, 2024 08:55
@jukzi
Copy link
Contributor

jukzi commented Feb 2, 2024

i rebased the commit to master, to get the tests running. just ping me if should forget to review the result

@erik-brangs
Copy link
Contributor Author

@jukzi The tests have completed, please review

@erik-brangs erik-brangs force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch from 49c0b80 to 6672a95 Compare February 2, 2024 11:38
@jukzi
Copy link
Contributor

jukzi commented Feb 2, 2024

ok, tests are run and don't fail:

[2024-02-02T09:05:10.728Z] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.419 s -- in AutomatedSuite org.eclipse.jdt.junit.launcher.JUnitLauncherTests org.eclipse.jdt.junit.launcher.AdvancedJUnitLaunchConfigurationDelegateTest
[2024-02-02T09:05:10.728Z] org.eclipse.jdt.junit.launcher.AdvancedJUnitLaunchConfigurationDelegateTest.runTestsInSourceFolderOnlyUsesTopLevelClasses -- Time elapsed: 0.176 s
[2024-02-02T09:05:10.729Z] org.eclipse.jdt.junit.launcher.AdvancedJUnitLaunchConfigurationDelegateTest.runTestsInSourceFolderHandlesMultiplePackagesInASourceFolderCorrectly -- Time elapsed: 0.090 s
[2024-02-02T09:05:10.729Z] org.eclipse.jdt.junit.launcher.AdvancedJUnitLaunchConfigurationDelegateTest.runTestsInSourceFolderHandlesMultipleSourceFoldersCorrectly -- Time elapsed: 0.085 s
[2024-02-02T09:05:10.729Z] org.eclipse.jdt.junit.launcher.AdvancedJUnitLaunchConfigurationDelegateTest.runTestsInSourceFolderHandlesMultipleSourceFilesInPackagesInASourceFolderCorrectly -- Time elapsed: 0.068 s

@jukzi
Copy link
Contributor

jukzi commented Feb 2, 2024

@Bananeweizen can you please review, as you have created the issue - does it solve your issue?

@erik-brangs erik-brangs force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch from 6672a95 to f57326d Compare February 2, 2024 12:03
@jukzi jukzi dismissed their stale review February 14, 2024 14:18

comment removed

@jukzi jukzi force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch from f57326d to 5f268ae Compare February 14, 2024 14:18
@jukzi
Copy link
Contributor

jukzi commented Feb 14, 2024

@Bananeweizen ping

@erik-brangs erik-brangs force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch 3 times, most recently from 11c8317 to 96f8718 Compare March 4, 2024 15:42
@erik-brangs
Copy link
Contributor Author

@jukzi Can this go in for 2024-06 or do I still need to do something?

@erik-brangs erik-brangs force-pushed the issue-920-test-runner-does-not-handle-source-folders-correctly branch from 96f8718 to 300c665 Compare March 6, 2024 09:29
@jukzi
Copy link
Contributor

jukzi commented Mar 6, 2024

the reported API errors are unrelated and i don't know why they are shown as new here
image

@iloveeclipse
Copy link
Member

the reported API errors are unrelated and i don't know why they are shown as new here

Is the commit based on master head?

@jukzi jukzi merged commit 87b98a6 into eclipse-jdt:master Mar 6, 2024
6 of 9 checks passed
@Bananeweizen
Copy link
Contributor

Sorry for completely missing the notifications here. That looks good to me. Thanks a lot for the change!

@iloveeclipse
Copy link
Member

It seems this PR caused regression, please check: #1651

@erik-brangs
Copy link
Contributor Author

I'll take a look but I can't say when. I have unreliable internet access at the moment. I don't know when I'll next have access that is good enough to do any work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants