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

LUCENE-10531: Add @RequiresGUI test group for GUI tests #893

Merged
merged 35 commits into from May 18, 2022

Conversation

mocobeta
Copy link
Contributor

@mocobeta mocobeta commented May 16, 2022

Mark TestScripts.testLukeCanBeLaunched as @RequiresGUI and add an Actions workflow for distribution tests that runs all distribution tests including GUI tests. The workflow should run on all major OSs: Linux, MacOS, and Windows.

On mandatory test runs:

./gradlew -p lucene/distribution.tests/ test -Dtests.gui=false
:lucene:distribution.tests:test (SUCCESS): 8 test(s), 1 skipped  // TestScripts is skipped.

On test runs where tests.gui sysprop is set to true:

./gradlew -p lucene/distribution.tests/ test -Dtests.gui=true
:lucene:distribution.tests:test (SUCCESS): 8 test(s)

On CI build, tests.gui sysprop is forced to set true to run GUI tests.

@Inherited
@Retention(RetentionPolicy.RUNTIME)
@TestGroup(enabled = false, sysProperty = SYSPROP_NIGHTLY)
public @interface Nightly {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Off-the-shelf com.carrotsearch.randomizedtesting.annotations.Nightly would be okay (tests.nightly system property is used for the test group as default), but I re-defined Nightly annotation here as LuceneTestCase does so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename the annotation "RequiresGui" or something like this. It really is not a nightly test - just one that has additional requirements/ side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right... but I think if we introduce a new test group and system property, Jenkins and smoke tester configurations also need to be tweaked?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning up those workflow files, @mocobeta ! I don't haven that much experience with github actions so it's definitely cruft that accumulated over time.

Adding a new test group itself (disabled by default) doesn't require any special actions. If we want to run it on nightly runs then yes - jenkins would have to be configured properly for this... Alternatively, the value of the property triggering the test group could be computed inside gradle scripts (much like Uwe recently modified the errorprone to run on CI servers). Does this sound better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. I'd like to run this test on the Jenkis Server with other nightly tests, and also in the smoke tester before every release.
I'll look at the alternatives once I will have made it work on Windows VM (currently it works only on Unix-like shells, of course).

Copy link
Contributor Author

@mocobeta mocobeta May 16, 2022

Choose a reason for hiding this comment

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

@dweiss there will be lots of retries to make it work on both Linux shell and Windows command (I don't have other ideas to test Actions); I think "Unsubscribe" this PR is recommended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dweiss I renamed the annotation (test group) as suggested, added tests.gui system property for the test group, and made the sysprop forcibly set to true on CI builds - does this change make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also corrected the pull request description to reflect the renaming.

@mocobeta
Copy link
Contributor Author

The added workflow takes 3 minutes on ubuntu-latest, and it takes 5 minutes on macos-latest.

@mocobeta
Copy link
Contributor Author

This worked on Windows VM - looks fairly slow (the whole workflow takes 7.5 minutes) as compared to Ubuntu (3 minutes) though.

@mocobeta
Copy link
Contributor Author

surprisingly, it worked on the first try... I'll open this and try to address #893 (comment)

@mocobeta mocobeta marked this pull request as ready for review May 16, 2022 15:21
@mocobeta mocobeta changed the title LUCENE-10531: Run scripts test nightly only LUCENE-10531: Run GUI tests on CI only May 16, 2022
Comment on lines 57 to 60
if (isCIBuild) {
// force to run GUI tests
args += ["-Dtests.gui=true"]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simply use isCIBuild variable that was introduced in #890; this might be hacky?

@mocobeta
Copy link
Contributor Author

mocobeta commented May 16, 2022

while waiting for review... here's Randomizedtesting Duke, rough-and-tumble boy (based on the "Tumble Duke") - the latest addition to my fan-art collection
duke_randomizedtesting

@mocobeta
Copy link
Contributor Author

The previous CI run result looks good to me. I also enabled GUI tests in the smoke tester.

@mocobeta
Copy link
Contributor Author

How about renaming the action's directory name .github/actions/yarn-caches/ to .github/actions/gradle-caches/? @dweiss

@dweiss
Copy link
Contributor

dweiss commented May 17, 2022

up to you, entirely. :)

@dweiss
Copy link
Contributor

dweiss commented May 17, 2022

The yarn-caches name is wrong - it's something else I was working on (!).

@mocobeta
Copy link
Contributor Author

I'll update the directory name later.

This time, the test timed out on Windows... I think this could occasionally happen :/

@dweiss
Copy link
Contributor

dweiss commented May 17, 2022

Increase the timeout, maybe? Windows boxes on github are slow.

@mocobeta
Copy link
Contributor Author

I increased the timeout to 120 seconds.

@mocobeta
Copy link
Contributor Author

sorry couldn't resist.

"Build Duke"
build_duke

@mocobeta
Copy link
Contributor Author

mocobeta commented May 17, 2022

Passed all checks and I think we've done all I wanted to do here - we disabled the GUI test in the mandatory test runs, instead, we enabled it on nightly CI runs (Jenkins, GH Actions) and the smoke tester.

@mocobeta
Copy link
Contributor Author

I'm merging this only to main - let me know if it's worth backporting.

@mocobeta mocobeta merged commit b911d1d into apache:main May 18, 2022
@mocobeta mocobeta deleted the run-scripts-test-nightly branch May 18, 2022 00:26
@dweiss
Copy link
Contributor

dweiss commented May 18, 2022

I'd apply this to 9x as well since it'll ease backports of other things/ decrease the potential of a conflict in the future?

@mocobeta
Copy link
Contributor Author

Ok I'll backport it to the 9x branch.

msokolov pushed a commit to msokolov/lucene that referenced this pull request May 20, 2022
Co-authored-by: Dawid Weiss <dawid.weiss@carrotsearch.com>
@mocobeta
Copy link
Contributor Author

We increased the timeout to 120 seconds, but the test still occasionally fails on Windows. I opened #917 to disable it on windows for now.

shaie pushed a commit to mdmarshmallow/lucene that referenced this pull request Jun 22, 2022
Co-authored-by: Dawid Weiss <dawid.weiss@carrotsearch.com>
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.

None yet

2 participants