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

Partest tests can require a java version range #9587

Merged
merged 3 commits into from
Apr 23, 2021

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Apr 22, 2021

... using a // javaVersion: N / N+ / N - M comment in the test soruce.
The test is skipped if the java version is outside the range.

@scala-jenkins scala-jenkins added this to the 2.13.6 milestone Apr 22, 2021
@SethTisue SethTisue added the internal not resulting in user-visible changes (build changes, tests, internal cleanups) label Apr 22, 2021
@lrytz lrytz requested a review from som-snytt April 22, 2021 14:30
@som-snytt
Copy link
Contributor

Ha, already looked at this when I woke up. Wondering if it means we can get rid of the "pragma" on the check file: control the version on the input side, not the output.

@lrytz
Copy link
Member Author

lrytz commented Apr 22, 2021

I didn't add support for java version ranges to keep it simple, but that would also be a possibility. Getting rid of the pragmas in check files would then mean duplicating the test, which doesn't gain us much I think...

@som-snytt
Copy link
Contributor

--update-check doesn't respect pragmas in check files, and probably never will.

There are only a few tests that are version or platform dependent. Maybe instead of range, just jdk: 11 17 where someone testing locally with 16 will see a skipped test, which is reasonable.

@lrytz
Copy link
Member Author

lrytz commented Apr 22, 2021

I agree that maintaining .check files with pragmas is a huge pain. Though switching jvm for a "quick" --update-check will be not so quick either :-)

I can do version ranges.

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

toolArgsForTheWin

@lrytz
Copy link
Member Author

lrytz commented Apr 23, 2021

Changed to support java version ranges, also added some doc to CONTRIBUTING.md

... using a `// javaVersion: N / N+ / N - M` comment in the test soruce.
The test is skipped if the java version is outside the range.
@lrytz lrytz marked this pull request as ready for review April 23, 2021 13:16
@lrytz lrytz changed the title Partest tests can require a minimal java version Partest tests can require a java version range Apr 23, 2021
@lrytz lrytz merged commit d112b8c into scala:2.13.x Apr 23, 2021
@harpocrates
Copy link
Contributor

Thank you so much for adding this, and so promptly too. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal not resulting in user-visible changes (build changes, tests, internal cleanups)
Projects
None yet
5 participants