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

Chore: remove memory test from CI #3758

Merged
merged 5 commits into from
Jun 28, 2022
Merged

Chore: remove memory test from CI #3758

merged 5 commits into from
Jun 28, 2022

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Jun 28, 2022

Changes

Remove our memory smoke test from CI only (test still present and manually runnable)

Why?

  • after speaking with @natemoo-re, it sounds like this memory test addressed out-of-date concerns for the compiler
  • this test causes issues with Vite 3, which relies on resolving the astro package to force-optimize dependencies. Since this test imports and calls the dev command directly (rather than using execa), Vite fails to resolve astro as an installed dependency. We can refactor this test to use execa instead if we feel this test is necessary!

Testing

Remove mk.js + command call from the CI

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Jun 28, 2022

🦋 Changeset detected

Latest commit: db8802c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
astro-scripts Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions github-actions bot added the 🚨 action Modifies GitHub Actions label Jun 28, 2022
@FredKSchott
Copy link
Member

If it’s not bothering anyone, can we leave the script in the repo? I remember this randomly being useful a couple of times when I was tracking down perf issues.

@bholmesdev
Copy link
Contributor Author

@FredKSchott This test is breaking Vite 3 (see PR comments) but maybe we can refactor to use execa instead? I'm not sure if that breaks the spirit of these tests

@FredKSchott
Copy link
Member

It’s not just a test! You can run the script yourself and it starts in a “watch” mode.

To be super clear, +1000 on removing from CI. But would love to keep the script for personal/maintainer use

@bholmesdev
Copy link
Contributor Author

@FredKSchott Ah gotcha! Removing from the CI would be perfect, yes 👍

@bholmesdev bholmesdev requested a review from FredKSchott June 28, 2022 19:01
@bholmesdev bholmesdev changed the title Chore: remove memory test Chore: remove memory test from CI Jun 28, 2022
@bholmesdev
Copy link
Contributor Author

bholmesdev commented Jun 28, 2022

Update: looks like manually running outside the CI also breaks with Vite 3... experimenting with a refactor to use execa instead

Second update: Vite might be removing this hard bomb from Vite 3. Logged an issue for discussion. Removing this test from our CI for now though since y'all agree!

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