Skip to content

Conversation

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 22, 2020

The test targets in build.gradle are set up to run only classes whose name ends with exactly Test. A test class named RunMyTest.java will be run, but a test called CheckSomething.java will not be. This was done because gradle's built-in scanner for test classes was buggy in earlier versions of gradle. It seems to have been fixed in the gradle 3.x time frame from what I can tell.

The current approach leads to two problems. The first is that gradle will try to execute all classes named XxxxxTest.java whether or not it should. Attempting to execute utility classes without test methods (meaning without at least one method annotated with @Test) or abstract classes (whether or not there are any test methods) will fail.

The second is that a test class that isn't named XxxxxTest.java will not be run, even if it contains @Test methods. This can (and has) lead to tests being skipped when they should be run.

The solution is to enable gradle's built-in scanning for test classes which works exactly like you would expect it to: it runs any concrete class that has at least one @Test method or whose parent class has such a method. This means that a class with no test methods that subclasses an abstract class with @Test methods will be run correctly.

I ran a full test on all three platforms. The following test classes which were formerly not run are now run:

modules/javafx.base: test.javafx.collections.ObservableListWithExtractor : 52 tests
modules/javafx.base: test.javafx.event.EventSerializationEventExists 0 tests (*)
modules/javafx.controls:. test.javafx.scene.control.MiscellaneousTests : 3 tests
tests/system: test.com.sun.javafx.application.SwingNoExit : 1 test

They pass on all three platforms.

(*) - There is only one test in this class and it is currently @Ignored


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8254013: gradle test should run all test classes even if they don't end with "Test"

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/329/head:pull/329
$ git checkout pull/329

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 22, 2020

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Oct 22, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 22, 2020

Webrevs

@kevinrushforth kevinrushforth requested a review from arapte October 23, 2020 11:37
Copy link
Member

@arapte arapte left a comment

Choose a reason for hiding this comment

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

Looks good to me, verified that tests in files which are not suffixed with Test get executed now.
buildSrc/build.gradle file also has the code that we are changing here,

scanForTestClasses = false
include "**/*Test.*"

I think it won't affect as there are no tests there.

@openjdk
Copy link

openjdk bot commented Oct 28, 2020

@kevinrushforth This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8254013: gradle test should run all test classes even if they don't end with "Test"

Reviewed-by: arapte, pbansal

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 7 new commits pushed to the master branch:

  • 690d266: 8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException
  • 2c67555: 8213573: MouseLocationOnScreenTest fails intermittently
  • 243b1a5: 8255241: [TestBug] Re-enable few ignored tests in javafx.controls module that pass with latest code
  • 4e5f0e6: 8252596: [TESTBUG] WebPageShim::paint is not thread-safe
  • a5a71d1: 8247494: Test failure in ImageRaceTest on some systems
  • 16b697c: 8254100: FX: Update copyright year in docs, readme files to 2021
  • ae1fb61: 8255002: Many javafx.controls unit tests have incorrect name containing impl_*

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Oct 28, 2020
@kevinrushforth
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Oct 28, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Oct 28, 2020
@openjdk
Copy link

openjdk bot commented Oct 28, 2020

@kevinrushforth Since your change was applied there have been 7 commits pushed to the master branch:

  • 690d266: 8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException
  • 2c67555: 8213573: MouseLocationOnScreenTest fails intermittently
  • 243b1a5: 8255241: [TestBug] Re-enable few ignored tests in javafx.controls module that pass with latest code
  • 4e5f0e6: 8252596: [TESTBUG] WebPageShim::paint is not thread-safe
  • a5a71d1: 8247494: Test failure in ImageRaceTest on some systems
  • 16b697c: 8254100: FX: Update copyright year in docs, readme files to 2021
  • ae1fb61: 8255002: Many javafx.controls unit tests have incorrect name containing impl_*

Your commit was automatically rebased without conflicts.

Pushed as commit 4f1eb7d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@kevinrushforth kevinrushforth deleted the 8254013-test-classes branch October 29, 2020 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.

3 participants