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

Update flakiness script #30

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Update flakiness script #30

wants to merge 9 commits into from

Conversation

outofambit
Copy link
Collaborator

@outofambit outofambit commented Aug 28, 2024

  • removed checking for empty responses
  • use a more markdown centric output format
  • group output by test ids
  • use a most common set of responses as the comparison set, instead of just using the first set of responses

@outofambit outofambit marked this pull request as ready for review September 5, 2024 18:18
outofambit and others added 3 commits September 5, 2024 15:29
* WIP: markdown reformatting progress

* fix for header section

* some changes based on carmen feedback

* Total unequal % - carmen request

* remove old code comment

* remove old code comment

* link to header

* adding some additional notes to stress test readme

* add notes about using a personal non-network fork

* simplify join in formatResponses

* remove old settle tracking

we got this from p-limit

* Update stressor/stress-test.mts

* Apply suggestions from code review

Co-authored-by: Mx Corey Frang <[email protected]>

---------

Co-authored-by: cypress evelyn masso <[email protected]>
@outofambit
Copy link
Collaborator Author

@jugglinmike if you would take a look at this, that would be great! @gnarf already took a look and added a lot of great improvements via #34.

@outofambit outofambit removed the request for review from gnarf September 26, 2024 18:26
Copy link
Member

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

Between the need to silence npm's output and the parameterized source code, I think this script now warrants a CLI. Could you make a follow-up task for that?


Running the script can take a while, as it is constrained by GitHub Actions availability and speed.
1. It is prefered for you to run the stress test against your own personal "non-fork" of this repo (create a personal repo and push to it instead of using "fork" so it isn't part of the "network") to limit the number of action runs against the main branch.
2. Update the stress-test.mts file `owner`, `repo`, and `defaultBranch` definitintions near the top, as well as setting up the tests / matrix you want to test.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
2. Update the stress-test.mts file `owner`, `repo`, and `defaultBranch` definitintions near the top, as well as setting up the tests / matrix you want to test.
2. Update the stress-test.mts file `owner`, `repo`, and `defaultBranch` definitions near the top, as well as setting up the tests / matrix you want to test.

Running the script can take a while, as it is constrained by GitHub Actions availability and speed.
1. It is prefered for you to run the stress test against your own personal "non-fork" of this repo (create a personal repo and push to it instead of using "fork" so it isn't part of the "network") to limit the number of action runs against the main branch.
2. Update the stress-test.mts file `owner`, `repo`, and `defaultBranch` definitintions near the top, as well as setting up the tests / matrix you want to test.
3. Run it with `npm run --slient stress-test | tee some-output-file.md`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
3. Run it with `npm run --slient stress-test | tee some-output-file.md`.
3. Run it with `npm run --silent stress-test | tee some-output-file.md`.

return testCombosForTestPlan.map(spawnAndCollectWorkflows);
}));

clearInterval(logStatusInterval);
Copy link
Member

Choose a reason for hiding this comment

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

Mind putting this in a finally block?

Comment on lines +326 to +328
const isRowEqual =
JSON.stringify(resultResponses) ===
JSON.stringify(baselineResponses);
Copy link
Member

Choose a reason for hiding this comment

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

Object serialization semantics being what they are, this seems a bit fragile to me. What do you think of writing a comparator function for this?

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.

3 participants