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

Refactor smoke tests to use submodules #2702

Merged
merged 8 commits into from
Mar 2, 2022
Merged

Conversation

natemoo-re
Copy link
Member

@natemoo-re natemoo-re commented Mar 2, 2022

Changes

  • Follow-up to Make smoke tests more deterministic #2618 which added our smoke tests inline to the repo. This caused a lot of trouble with rebasing because all commits and changes are tracked for these directories, even though they have their own commit flow.
  • Uses git submodules to manage the astro.build and docs smoke tests. These will only be fetched for the smoke test action and will always fetch the latest commit to main. It's automatic and doesn't require an action to sync these nightly.
  • Contributors shouldn't have trouble with this because git pull won't fetch submodules by default.

Testing

CI

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Mar 2, 2022

⚠️ No Changeset found

Latest commit: 57a4e6a

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 Mar 2, 2022
Comment on lines +181 to +183
uses: actions/checkout@v3
with:
submodules: 'recursive'
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the extent of the changes to our actions. This pulls in the submodules recursively, so they always reflect the latest commit to main every time they run.

Comment on lines +1 to +8
[submodule "smoke/docs"]
path = smoke/docs
url = [email protected]:withastro/docs.git
branch = main
[submodule "smoke/astro.build"]
path = smoke/astro.build
url = [email protected]:withastro/astro.build.git
branch = main
Copy link
Member Author

Choose a reason for hiding this comment

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

This points ./smoke/docs to [email protected]:withastro/docs.git and smoke/astro.build to [email protected]:withastro/astro.build.git. They both should reflect the main branch.

By default, git pull doesn't populate these directories unless you explicitly opt-in to that behavior (which most users won't need to do)

Copy link
Member Author

Choose a reason for hiding this comment

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

I should mention that users won't do this often, but our smoke test action can specifically populate them just for that run. Seems much better than the previous system!

@matthewp
Copy link
Contributor

matthewp commented Mar 2, 2022

Needs to be rebased :(

@natemoo-re natemoo-re force-pushed the refactor/submodules branch from 23d4d8d to fbceb7d Compare March 2, 2022 20:23
@FredKSchott
Copy link
Member

FredKSchott commented Mar 2, 2022

@natemoo-re from my comment here: #2618 (comment)

I'd still love to try our current system for another week, but if it's already been a pain for maintainers in so little time then I'm +1 on this PR. I'm just surprised that this has impacted rebasing since we should never be touching that directory... but I guess they still find their way into the rebase diff?

Regardless, +1 if you feel strongly.

One thing to watch out for, that I realized in the smoke sync script: checking out these git submodules locally and then re-running yarn will produce a different yarn.lock than you'd expect. If we can assume that only advanced maintainers are running smoke tests locally, then I guess we could just document this somewhere or remember to keep an eye out for it when reviewing PRs.

@natemoo-re
Copy link
Member Author

Thanks @FredKSchott! Yeah unfortunately the automatic sync PR merge commits still get picked up when you rebase from main, so you have to sort through all of those changes on top of whatever you're actually changing.

I'll give the yarn.lock issue a once-over before merging this.

@natemoo-re natemoo-re merged commit 2482fe7 into main Mar 2, 2022
@natemoo-re natemoo-re deleted the refactor/submodules branch March 2, 2022 22:08
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* chore: delete inlined repos

* refactor: move smoke tests to submodules

* chore: remove smoke sync action

* chore: update ci to fetch submodules for smoke test only

* chore: fix ci script

* feat: delete inlined smoke tests

* fix: update lockfile to exclude smoke tests

* chore(ci): ensure smoke tests can pass in CI
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.

3 participants