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

test: fix run css playground with legacy sass #18946

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

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Dec 12, 2024

Description

Built on top of #18945

Since #18636, the legacy sass tests for playground/css was not run because playground/css/__tests__/sass-legacy/sass-legacy.spec.ts was missing. This PR adds that.

Also this PR extracts sass related tests in a different file to skip running other tests for sass-legacy.spec.ts and sass-modern.spec.ts.

old description

Also this PR does the following changes to reduce flaky fails.

  • makes HMR related tests to be only run in css.spec.ts
    • Without this change, all css.spec.ts and sass-modern.spec.ts and sass-legacy.spec.ts tries to change the file and fails sometimes.
  • make sass related tests to expect either the expected value before HMR or the expected value after HMR
    • Without this change, the test fails when css.spec.ts changes the file and other tests observes the changed file.
  • only run sass related tests in sass-*.spec.ts
    • This is just to reduce the places that needs to write expectToBeEither and also to reduce duplicated test runs.

@sapphi-red sapphi-red added p1-chore Doesn't change code behavior (priority) test labels Dec 12, 2024
@sapphi-red sapphi-red marked this pull request as draft December 12, 2024 02:52
@sapphi-red sapphi-red marked this pull request as ready for review December 16, 2024 12:38
Copy link
Member

@bluwy bluwy 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 not really sure about expectToBeEither, it feels like it could make tests harder to debug if it fails 🤔 Should the HMR tests do cleanup instead so it returns to original? Are the tests also running serially too?

@sapphi-red
Copy link
Member Author

sapphi-red commented Dec 17, 2024

Should the HMR tests do cleanup instead so it returns to original? Are the tests also running serially too?

The tests (css.spec.ts, sass-modern.spec.ts, sass-legacy.spec.ts) are running in parallel because they are different files. So adding a cleanup won't solve the problem here. I thought about only running these test files serially, but didn't find a way to do it.

@hi-ogawa
Copy link
Collaborator

the test fails when css.spec.ts changes the file and other tests observes the changed file.

Does adding css-legacy to this help? I think I made this to separate sources between test variants.

// also setup dedicated copy for "variant" tests
for (const [original, variants] of [
['css', ['sass-modern', 'sass-modern-compiler']],
['css-sourcemap', ['sass-modern', 'sass-modern-compiler']],

@sapphi-red
Copy link
Member Author

Oh, I wasn't aware of that! Let me try that.

@sapphi-red
Copy link
Member Author

Yep, it works 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-chore Doesn't change code behavior (priority) test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants