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

Ci flake finder for unit tests #16279

Merged
merged 3 commits into from Apr 27, 2024
Merged

Conversation

mikehardy
Copy link
Member

Purpose / Description

We had a hard time with windows unit test flake reproduction.
I threw in a crude hack on the workflow as a temporary commit that iterated over windows a lot of times, and that allowed for reproduction confidence

This is an attempt to make that a general feature - so you can select either all 3 operating systems we run or focus on one of them, and you can run an arbitrary number of iterations of unit tests on it (okay, 255 limit, but that's a lot)

Fixes

Approach

  • first make our matrix generation dynamic
  • second make operating system default to all 3 like normal but allow selection if desired
  • third add idea of iterations to unit tests, and allow specification of more than 1 if desired

How Has This Been Tested?

On my separate fork: https://github.com/mikehardy/Anki-Android/actions/workflows/tests_unit.yml

  • I made sure that the "push" case was normal, it ran all 3 systems with one iteration
  • I made sure you could select an operating system and it would run just that one
  • I made sure you could specify an iteration count and it would add iterations if not just 1

Learning (optional, can help others)

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

this will allow us to have different matrix expansions for different
use cases, will be used to add iterative expansion on infrequent schedule
or on workflow dispatch with parameters to add statistical power to
flake-hunting attempts
@mikehardy mikehardy added Dev Development, testing & CI CI labels Apr 26, 2024
@mikehardy
Copy link
Member Author

Each commit works by itself, I had to build this very very carefully since the merger of matrices + yaml + javascript + json is really touchy. So I went slowly and each commit was tested by itself for correct functionality - rebase merge will be fine

@mikehardy
Copy link
Member Author

mikehardy commented Apr 26, 2024

Note: will require a settings change such that branch protections are now looking for the required status from the 3 runs but with "Unit Tests / " prepended to the check names

Also has run count in there, so perhaps the branch protection can be a regex 🤔 - investigating

Okay: investigated

We will need to temporarily disable the specific status checks for unit tests while this goes in
Then after one set of runs we can re-add required status checks as:

  • Unit Tests / JUnit Tests (macos run 1)
  • Unit Tests / JUnit Tests (ubuntu run 1)
  • Unit Tests / JUnit Tests (windows run 1)

That should work because we want to see at least 1 iteration of each, and that is the default set produced from a pull request event trigger on this workflow

@mikehardy
Copy link
Member Author

@krmanik just tagging you because you might be interested in this - you've done so much github workflow + github-script and it's a potential useful trick to know how to get inputs into the javascript in a usable way, and use javascript to pass information to future steps/jobs

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

documentation-only. Code looks good

.github/workflows/tests_unit.yml Outdated Show resolved Hide resolved
.github/workflows/tests_unit.yml Show resolved Hide resolved
.github/workflows/tests_unit.yml Show resolved Hide resolved
.github/workflows/tests_unit.yml Outdated Show resolved Hide resolved
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Implementer's choice on all comment trees. Let's get this in

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) labels Apr 26, 2024
this should allow us to get statistical confidence that we can
both reproduce flakes and say we have fixed them
@mikehardy
Copy link
Member Author

I liked the suggestions - had just enough time for the docs polishes and re-pushed, re-tested on my branch, the default expansion was still as expected (all 3, 1 iteration) and a manual run of windows + 3 worked, both visible here https://github.com/mikehardy/Anki-Android/actions/workflows/tests_unit.yml

@mikehardy mikehardy removed the Needs Author Reply Waiting for a reply from the original author label Apr 27, 2024
@mikehardy
Copy link
Member Author

I hate to merge this then ghost so I'm going to wait until I have time or someone else can commit to the branch protection temp disable / re-enable with new names I mention above with link to settings area #16279 (comment)

@krmanik
Copy link
Member

krmanik commented Apr 27, 2024

@krmanik just tagging you because you might be interested in this - you've done so much github workflow + github-script and it's a potential useful trick to know how to get inputs into the javascript in a usable way, and use javascript to pass information to future steps/jobs

The input can be provided similar to

var inputs = ${{ toJSON(inputs) }}

We can use @actions/core to set output.

core.setOutput('outputKey', 'outputVal');

To access the value in same job,

${{ steps.[STEP ID].outputs.[KEY] }}

To access the value in different job,

${{ needs.[JOB ID].outputs.[KEY] }}

Defining outputs for jobs

@mikehardy
Copy link
Member Author

The toJSON I used but still need to parse the os array because it was still a string inside inputs

The output was just using default return from the script so didn't need anything fancy theree

@mikehardy
Copy link
Member Author

Okay, I'm going to merge this now then do the required baby-sitting on branch protection rules

@mikehardy mikehardy added this pull request to the merge queue Apr 27, 2024
Merged via the queue into ankidroid:main with commit e524c8f Apr 27, 2024
6 checks passed
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Apr 27, 2024
@github-actions github-actions bot added this to the 2.18 release milestone Apr 27, 2024
@mikehardy
Copy link
Member Author

New branch protection rules are in place.
Existing PRs will need a close/reopen cycle to kick off the new CI tests expected by the new rules

@mikehardy mikehardy deleted the ci-flake-finder branch April 27, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Dev Development, testing & CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants