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.

Sign up for GitHub

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

Jump to bottom

Enhance build matrix in GitHub Actions #2584

Merged
merged 13 commits into from
Jun 11, 2021
Merged

Enhance build matrix in GitHub Actions #2584

merged 13 commits into from
Jun 11, 2021

Conversation

vlsi
Copy link
Contributor

@vlsi vlsi commented Jun 4, 2021

This PR enables adding new test dimensions while keeping CI duration under control.

Issues identified:

  • testng build does not work with OpenJ9 JVM (looks like com.sun.management.ThreadMXBean does not exist there)
  • testng does not work with XX:hashCode=2
  • test.yaml.YamlTest > testXmlDependencyGroups sometimes produces a trailing whitespace (windows-related issue?)

@vlsi
Copy link
Contributor Author

vlsi commented Jun 4, 2021

Frankly speaking, I am inclined to rename Java CI with Gradle to Test to make GitHub UI look better.

@vlsi vlsi force-pushed the build_matrix branch 3 times, most recently from ec1ab7f to a0c8f22 Compare June 5, 2021 00:06
@juherr
Copy link
Member

juherr commented Jun 5, 2021

Frankly speaking, I am inclined to rename Java CI with Gradle to Test to make GitHub UI look better.

Lgtm

@vlsi vlsi force-pushed the build_matrix branch 9 times, most recently from 9c8368e to db00051 Compare June 5, 2021 12:30
@vlsi
Copy link
Contributor Author

vlsi commented Jun 5, 2021

I guess the PR is ready.

There is a couple of flaky tests left #2587 and #2586, however, I do not understand what do they test and why do they fail.

juherr
juherr reviewed Jun 6, 2021
View reviewed changes
Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

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

Really cool!

.github/workflows/matrix.js Show resolved Hide resolved
.github/workflows/matrix.js
// Zulu
{group: 'Zulu', version: '8', distribution: 'zulu'},
{group: 'Zulu', version: '11', distribution: 'zulu'},
{group: 'Zulu', version: '16', distribution: 'zulu'},
Copy link
Member

Choose a reason for hiding this comment

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

We are only supporting the 2 latest LTS (8 + 11 and 11 + 17 soon).

Is it possible to have the other version as optional? We did it with Travis and it helps to have an eye on potential issues without breaking the build.
Is it possible to have EAP version as optional? https://jdk.java.net/17/ It will allow us to anticipate issues without breaking the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gradle does not support running with Java 17 yet, so building with Java 17 needs to configure toolchain for the build: https://docs.gradle.org/7.0.2/userguide/toolchains.html

.github/workflows/matrix.js Show resolved Hide resolved
settings.gradle.kts Show resolved Hide resolved
testng-asserts/src/test/java/org/testng/AssertTest.java Outdated Show resolved Hide resolved
testng-core/src/main/java/org/testng/internal/Graph.java Show resolved Hide resolved
@vlsi vlsi requested a review from krmahadevan as a code owner June 6, 2021 08:48
krmahadevan
krmahadevan reviewed Jun 6, 2021
View reviewed changes
Copy link
Member

@krmahadevan krmahadevan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@vlsi - Is there any reference document that you can perhaps link to and create it as a markdown file so that anyone who wants to enhance this stuff but is not familiar with the matrix workings, can refer to and understand ?

Also please help fix the merge conflicts (I guess it got triggered due to the line endings PR that was merged ahead of this)

@vlsi
Copy link
Contributor Author

vlsi commented Jun 7, 2021

@krmahadevan , I've published the script as https://github.com/vlsi/github-actions-random-matrix
It might be good to move the script to npm in the future, however, the script is small, and having it in testng repository makes it easy to test locally. You can run node matrix.js and you get the matrix JSON.

@juherr
Copy link
Member

juherr commented Jun 8, 2021

Please rebase 🙏

@juherr
Copy link
Member

juherr commented Jun 10, 2021

@krmahadevan Is this PR ok for you? I think it will help us to find issues.

@krmahadevan
Copy link
Member

@krmahadevan Is this PR ok for you? I think it will help us to find issues.

@juherr Yes I am fine with this PR. I think there are merge conflicts which need to be fixed. I will explore the tool that @vlsi has shared to understand how it works.

vlsi added 3 commits June 11, 2021 17:53
@vlsi
Enhance build matrix in GitHub Actions
5bbc324 
The matrix selects random values for os, jdk, timezone to keep CI
duration reasonable.

Note: important combinations (e.g. os=windows, os=linux) are listed
explicitly, so PR builds are always verified for cross-platform compatibility.
@vlsi
Publish build scan only when --scan is explicitly passed
56719e9
@vlsi
test: avoid direct reference to com.sun.management.ThreadMXBean in te…
3e8471e 
…st code
vlsi added 10 commits June 11, 2021 17:55
@vlsi
Wrong results from GuiceBasedObjectDispenser when there's Object#hash…
868447b 
…Code collision
@vlsi
test: account for negative hashcodes in test.dataprovider.DataProvide…
8c4aa4b 
…rTest#instanceDataProviderTest
@vlsi
test: ignore newline differences in test.yaml.YamlTest#testXmlDepende…
0431405 
…ncyGroups
@vlsi
fix dataprovider
845a800
@vlsi
test: use IdentityHashMap to properly track test object instances
0b27a26 
intance.toString() is not guaranteed to be unique
@vlsi
test: use test object instances instead of their hashcodes in Invocat…
2400dfc 
…ionTracker
@vlsi
test: use class instance instead of its hashcode in BaseParallelizati…
c216183 
…onTest
@vlsi
Fix parallel and configfailurepolicy parsing in tr_TR locale
07a57bf 
"skip".toUpperCase() yields "SKİP"

See https://en.wikipedia.org/wiki/Dotted_and_dotless_I
@vlsi
Fix: use proper instances for beforeClass callback when different ins…
450ac95 
…tances collide on hash codes

It fixes test.factory.github2428.IssueTest#issue2438
@vlsi
Reduce the number of ITestNGMethod#toString invocations during the te…
e968c94 
…st run
@vlsi
Copy link
Contributor Author

vlsi commented Jun 11, 2021

@juherr , I rebased the PR.

@juherr juherr merged commit 0ace1d4 into testng-team:master Jun 11, 2021
@vlsi vlsi deleted the build_matrix branch June 20, 2021 13:45
@vlsi vlsi mentioned this pull request Jun 15, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juherr juherr juherr approved these changes

@krmahadevan krmahadevan krmahadevan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vlsi @juherr @krmahadevan