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

Feature: Adds parallel GitHub actions for e2e tests to get rid of flaky tests #4977

Merged
merged 15 commits into from
Dec 21, 2021

Conversation

wiardvanrij
Copy link
Member

@wiardvanrij wiardvanrij commented Dec 21, 2021

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

I believe quite a lot of our e2e tests simply hit a deadline due to -timeout 10m
Now, we could increase that, but I personally dislike builds > 10 min :)

This feature implements a pretty cool golang bin via gotesplit - which we fetch via scripts/gotesplit.sh. We place it in the /tmp/bin just like the Shellcheck bin.

What this tool does is it splits the tests based on the amount of parallelism you want. At the moment it does 0 on local dev work, which is exactly the same behavior as it already is. Mostly because our own computers do not benefit from the parallelism in our "runner", because that's our PC. We already do parallelism in the tests itself, which is perfect for that.

However for cloud based runners, it helps to have n-amount of runners, as it does not scale well vertically, but actually does it pretty good horizontally.

An example from my fork:

image

We increase the total amount of build minutes by roughly 40%, but we reduce the build time by 30-40% and actually remove quite a lot of flaky tests. Probably due to lack of GitHub runners and/or busy servers(?).

It can be further improved to actually cache the docker build stage so that each parallel testing job has access to the freshly prepared container image of Thanos.

Verification

Tested it locally and on my fork.

Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
@wiardvanrij
Copy link
Member Author

p.s. I have no idea why I had to format the docs, but.. yea it's included as well!

Signed-off-by: Wiard van Rij <wiard@outlook.com>
Makefile Outdated Show resolved Hide resolved
@GiedriusS
Copy link
Member

I wonder why this still has

Thanos end-to-end tests Expected — Waiting for status to be reported

Even though they have been removed :/

Failing docs check seems related to your changes. Why there are so many whitespace changes? 😄

GiedriusS
GiedriusS previously approved these changes Dec 21, 2021
@matej-g
Copy link
Collaborator

matej-g commented Dec 21, 2021

Regarding docs, if you ran make lint before committing, it clashes with make check-docs - both format in different ways but currently check-docs is the authoritative command since that's used by CI. See my forgotten PR here: #4662

matej-g
matej-g previously approved these changes Dec 21, 2021
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Looks good! (Minus doc check). Nice find with the Gotesplit 👍

@wiardvanrij
Copy link
Member Author

wiardvanrij commented Dec 21, 2021

@GiedriusS

The docs are actually better, but there is some quirk that the check-docs does not clear white noise.
I've talked with @matej-g and we are going to address this in #4662 were we reached consensus of how it should look like. That gets fixed tomorrow (or just soon :) )

In other words, the changed docs files are okay for now, even though it should not have been part of this PR. I think when we merged the other PR, it should have 0 diff anymore on it.

As for Bingo. I was implementing this, but hit this issue that gets fixed as well: bwplotka/bingo#106

Signed-off-by: Wiard van Rij <wiard@outlook.com>
@wiardvanrij wiardvanrij dismissed stale reviews from matej-g and GiedriusS via f23160b December 21, 2021 19:24
Signed-off-by: Wiard van Rij <wiard@outlook.com>
@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 21, 2021
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
@GiedriusS
Copy link
Member

Indeed it looks like you are correct - Thanos end-to-end tests is specified in the "checks that need to pass before merging" page and it probably waits forever for those results even though they will never come in this pull request. So, I will just adjust the list of those checks after merging this.

@GiedriusS GiedriusS merged commit 013a2e7 into thanos-io:main Dec 21, 2021
@GiedriusS
Copy link
Member

@wiardvanrij awesome work & a clever hack 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants