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

Make smoke tests more deterministic #2618

Merged
merged 2 commits into from
Mar 1, 2022
Merged

Make smoke tests more deterministic #2618

merged 2 commits into from
Mar 1, 2022

Conversation

FredKSchott
Copy link
Member

@FredKSchott FredKSchott commented Feb 18, 2022

Changes

  • Moves the actual download/setup/sync logic of our remote smoke tests to a nightly CI task. Instead of downloading on demand, we now download once nightly.
  • Now smoke tests should pass or fail with more reliability, since all test contents will static and checked in across every PR and test run. Essentially, we're back to "local" behavior for test running.
  • Uses the same "create PR" flow as our yarn.lock upgrade script.

Testing

  • Is test. I will run the nightly job manually after merging to confirm that it worked.

Docs

  • N/A

@changeset-bot
Copy link

changeset-bot bot commented Feb 18, 2022

⚠️ No Changeset found

Latest commit: dd355fc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the 🚨 action Modifies GitHub Actions label Feb 18, 2022
@natemoo-re
Copy link
Member

I don't know if we actually want these files in the repo and showing up in diffs?

@FredKSchott
Copy link
Member Author

FredKSchott commented Feb 19, 2022

If you think of the pros, it means that we can reliably reproduce our tests going all the way back in the git history, in case you need to track down when something broke. If a smoke test passes one day and fails the next, you can guarantee that it's because the code changed and not that there was some network issue, or that two people have two different sets of files downloaded on their machine. From experience, debugging that kind of thing can be a nightmare.

FWIW only the nightly job should ever be touching this directory in specific update commits that go directly onto main, so our own PR diffs should be relatively unaffected.

@natemoo-re
Copy link
Member

natemoo-re commented Feb 19, 2022

I know I jokingly say this all the time, but this actually seems like a case for submodules? When people clone this repo, they'll just get an empty file unless they pass --recurse-submodules, which is what we'd do in CI. More about Submodules. Seems kinda like this is what they were designed for.

@FredKSchott
Copy link
Member Author

Submodules would probably fix some bugs with our current implementation, but doesn't change the main pro/cons around determinism. Likewise, false positives/negatives are possible (likely?) in the current system due to network, fs, git, etc. Basically, if I see a smoke test failure (locally or CI) and you can't reproduce on your machine, there's a huge surface area to go debug whats wrong.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

I'm still not a big fan of this code being checked into our repo, especially since some contributors have already mentioned that the monorepo is difficult to clone on a slow connection. But I won't block this PR if you think this is the best approach for the time being.

@FredKSchott
Copy link
Member Author

Lets try it out for a few weeks and if we don't like it I'm happy to revert. I agree that a longer initial download time is a pain but I have faith that the confidence boost + being able to turn smoke tests back on will be well worth it

@FredKSchott FredKSchott merged commit 918f1ea into main Mar 1, 2022
@FredKSchott FredKSchott deleted the wip-smoke-2 branch March 1, 2022 05:38
natemoo-re pushed a commit that referenced this pull request Mar 1, 2022
* sync first remote smoke tests

* update smoke test scripts
natemoo-re pushed a commit that referenced this pull request Mar 2, 2022
* sync first remote smoke tests

* update smoke test scripts
natemoo-re pushed a commit that referenced this pull request Mar 2, 2022
* sync first remote smoke tests

* update smoke test scripts
natemoo-re pushed a commit that referenced this pull request Mar 2, 2022
* sync first remote smoke tests

* update smoke test scripts
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* sync first remote smoke tests

* update smoke test scripts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 action Modifies GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants