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

CODETOOLS-7903267: Support executing a single test method in a JUnit class #115

Conversation

jonathan-gibbons
Copy link
Collaborator

@jonathan-gibbons jonathan-gibbons commented Sep 9, 2022

Please review a medium-sized patch to support new feature that provides a way to specify that only parts of a test should be executed. For JUnit tests, this allows a single method in the test to be executed, but the underlying mechanism is more general than that and can be leveraged by any framework that supports running many test cases within an overall test.

The feature is enabled by using a new syntactic form on the command line, informally called query syntax: path-to-test?string. The syntax is based on URL syntax, but note that unlike URLs, the query component comes at the end, and not before any fragment component. In other words, the full syntax is path#id?string. Generally, this reflects the hierarchical structure: a path to a file, a test description within the file, and a string to specify part of the test to be run.

In terms of implementation, the "heavy lifting" is done in TestManager, to track the file, id, query tuple, as compared to the previous file, id. New abstractions TestSpec and GroupSpec are introduced, to replace stylistic use of simple strings.

The underlying JT Harness does not know (or need to know) about query components and so is unchanged. But the back end RegressionScript used to execute each test does need to know the query component, and so the information about which tests involve a query component is tunneled through the RegressionParameters in a side channel, embodied by a new resource in the object. In itself, the RegressionScript has little to do with the query component, it is simply passed down to the actions of the test in a new test.query property or TESTQUERY environment variable.

JUnitRunner is updated to check for the new test.query property: if set, it is used as the name of a test method to be run. Note: a JUnit exception will be thrown and the test will fail if the method is not found. The exception does contain enough useful information to diagnose the issue, but we may want to detect and report the issue in a more friendly form in a followup change.

Finally, new jtreg self-tests are added to exercise the new feature.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jtreg pull/115/head:pull/115
$ git checkout pull/115

Update a local copy of the PR:
$ git checkout pull/115
$ git pull https://git.openjdk.org/jtreg pull/115/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 115

View PR using the GUI difftool:
$ git pr show -t 115

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jtreg/pull/115.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 9, 2022

👋 Welcome back jjg! 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 Pull request is ready for review label Sep 9, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 9, 2022

Webrevs

Copy link
Member

@irisclark irisclark left a comment

Choose a reason for hiding this comment

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

Fantastic feature to allow developers to conveniently run specific tests during development!

I appreciate the addition documentation (javadoc and comments).

src/share/doc/javatest/regtest/faq.md Show resolved Hide resolved
src/share/doc/javatest/regtest/faq.md Outdated Show resolved Hide resolved
test/junitQueryTest/JUnitQueryTest.gmk Show resolved Hide resolved
test/junitQueryTest/JUnitQueryTest.gmk Show resolved Hide resolved
src/share/classes/com/sun/javatest/regtest/tool/Tool.java Outdated Show resolved Hide resolved
src/share/classes/com/sun/javatest/regtest/tool/Tool.java Outdated Show resolved Hide resolved
src/share/classes/com/sun/javatest/regtest/tool/Tool.java Outdated Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Sep 10, 2022

@jonathan-gibbons 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:

7903267: Support executing a single test method in a JUnit class

Reviewed-by: iris

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 Pull request is ready to be integrated label Sep 10, 2022
@LudwikJaniuk
Copy link

Does the new syntax allow skipping that part of the test? I.e., can I use the new granularity for exclusion, as well as inclusion?

@sormuras
Copy link
Member

sormuras commented Sep 12, 2022

As the title of this issue and pull request states, this new syntax only supports running one specific test (method) of a JUnit test (class).

A more general test selection and filter feature could be described and implemented in a separate feature request. Maybe supporting (a subset of) all JUnit Platform's selection and filter options: https://junit.org/junit5/docs/current/user-guide/#running-tests-console-launcher-options

@jonathan-gibbons
Copy link
Collaborator Author

As the title of this issue and pull request states, this new syntax only supports running one specific test (method) of a JUnit test (class).

A more general test selection and filter feature could be described and implemented in a separate feature request. Maybe supporting (a subset of) all JUnit Platform's selection and filter options: https://junit.org/junit5/docs/current/user-guide/#running-tests-console-launcher-options

Yes, but ...

The primary design center of the jtreg support for TestNG and JUnit to to bulk run these tests as part of a larger test run, perhaps as part of a test group or tier. The ability to run a single method is potentially interesting, for debugging purposes. The more that folk want to use more of the TestNG and JUnit features, the more they should be using those tools directly, or perhaps using an IDE, especially for "self-contained" tests. The exception is probably the tests that require features like native code libraries that is more conveniently provided by the "make test" feature in the makefiles.

@jonathan-gibbons
Copy link
Collaborator Author

As the title of this issue and pull request states, this new syntax only supports running one specific test (method) of a JUnit test (class).

While the motivation and code here is for JUnit, the mechanism is intended to be applicable to other test frameworks. There are a variety of such frameworks in the JDK test base, but we'll have to wait for this version of jtreg to be the default before we can use this feature in those frameworks.

@jonathan-gibbons
Copy link
Collaborator Author

Does the new syntax allow skipping that part of the test? I.e., can I use the new granularity for exclusion, as well as inclusion?

No, not at this time. It would be better to debug and fix the test method so that it does not need to be excluded!

However, as far as jtreg is concerned, there is no semantics to the query string; it is up to the test (runner) to interpret the string, and that could support an extended syntax, such as exclusion test-path?-method or a list test-path?m1,m2.
But I think we should wait to see a compelling use case for such forms.

Note also that you can always use JUnit Platform directly to execute a test, and with that, you have the full power of the JUnit platform command line.

Copy link
Member

@irisclark irisclark left a comment

Choose a reason for hiding this comment

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

Updates look good. Thanks!

@jonathan-gibbons
Copy link
Collaborator Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 13, 2022

Going to push as commit 7c7737d.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 13, 2022
@openjdk openjdk bot closed this Sep 13, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 13, 2022
@jonathan-gibbons jonathan-gibbons deleted the jtreg.7903267.test-manager-query branch September 13, 2022 19:53
@openjdk
Copy link

openjdk bot commented Sep 13, 2022

@jonathan-gibbons Pushed as commit 7c7737d.

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

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.

4 participants