Skip to content

Please, CI: review the test matrix #2575

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

Closed
Anton-Latukha opened this issue Jan 9, 2022 · 8 comments
Closed

Please, CI: review the test matrix #2575

Anton-Latukha opened this issue Jan 9, 2022 · 8 comments
Labels
CI Continuous integration status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet

Comments

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Jan 9, 2022

A while ago noticed that test matrix, lets say "needs to be reviewed".

MPJ noticed that also #2503 (comment)

(macOS never runs the test suite)


The matrix overall can be reviewed to do fewer builds & tests but do that strategically.

@Anton-Latukha Anton-Latukha added type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. status: needs triage labels Jan 9, 2022
@Anton-Latukha
Copy link
Collaborator Author

Current matrix:
image

@Anton-Latukha
Copy link
Collaborator Author

I also have a bias towards Linux, but not to that degree 8).

@Anton-Latukha
Copy link
Collaborator Author

Since changes generally relate/address the newest changes in GHCs - it is important to check the testsuite for the newest GHCs on all platforms.

The oldest one can be skipped on Linux, in favor of macOS, since Linux is generally more Linux enthusiasts & the platform is fastest at updating the packages & other things. This argumentation is minor, because a lot of test suite runs that are unlikely to fail from a set (for example Ubuntu Linux GHC 8.6.5, with that GHC 8.6.5 passed on other platform) - that check can be deferred to run on main branch after the merge. So minor checks would stop run constantly on PR push & run once after the merge to check minor possibilities "just in case". In case some of those minor checks is failing in the main branch - that workflow state does not block other PRs to be merged, that minor check only notifies maintainers/community on the test failure in particular GHC version. And in a majority of cases the maintainers can right away request from PR author that contributed the patch - to fix the build for that version of GHC, so the author sends the minor patch. The described process happens rarely, especially for example - for things that do not use low-level OS FFIs (for example plugins, if they are built on 1 platform - for them unlikely to fail on the other platforms).

@Anton-Latukha
Copy link
Collaborator Author

The very conservative way then would look like:
Screenshot-2022-01-09-22:07:49

@Anton-Latukha
Copy link
Collaborator Author

Anton-Latukha commented Jan 9, 2022

The more, for brevity, effective matrix would look something like:
image

The newest & recommended & the oldest GHCs run the testsuite on PR but run it in different platforms & then additionally cross-check results after the merge.

The after merge cross-checks are much cheaper, they are running once per merge (so much more checks can run) & nobody waits for them, we even can cover all GHCs & platforms with the after-merge checks. They are essentially to check less probable things through an ad-hoc system of notification.

@Anton-Latukha
Copy link
Collaborator Author

Anton-Latukha commented Jan 9, 2022

But this optimization has a value when the time can be saved, or project contribution flow exhausts the CI job runners.

As we see - I kept the 2 of 3 longest jobs - Windows test jobs, so in that case, the CI loop would not really be shorter.

That can be solved the other way:

Before merge we frequently do a PR rebase, which triggers CI anew.

We do not run the Windows test jobs at all during PR development. & rely on test reports from faster jobs from Linux & macOS.

When CI triggers & the merge me tag is present - all people walked away from the PR- it is when the CI runs the longest jobs - Windows test suite runs, and only if that required Windows job builds succeed - the requirement are met for the bot to merge the PR.

Otherwise in several hours someone from PR process notices the error return code from Windows tests & that gets addressed to pass PR into merge.

@Anton-Latukha
Copy link
Collaborator Author

Anton-Latukha commented Jan 9, 2022

That would be:

image

So initially - we were running 7 testsuite at the start, 3 of them Windows, the longest jobs.

As the result:

  • Now the test suite would also run on macOS,
  • in fact test suite would run on all platforms & all GHCs, it is 4 jobs per platform.
  • So, we have 12 testsuite jobs total.
  • And during that we are now actually saving the CI job runners.
  • And we are no longer waiting for the Windows builds to pass.
  • Since merge always requires a rebase onto main branch, the latency in merges (merge me checks) does not changes the number of rebases/CI build triggers inside PR is the same, so it does not change the process, except of having the probablity of some rare platform-specific bug found right after the merge, which can be addressed by the author or maintainer, but all dev CI loops become faster and we get wider test coverage that covers all combinations of platform & GHC.

@kokobd kokobd added CI Continuous integration status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet and removed type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. status: needs triage labels Sep 7, 2022
@michaelpj
Copy link
Collaborator

I did quite a bit on this, it's in a better state now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet
Projects
None yet
Development

No branches or pull requests

3 participants