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

Another way to allow leading space in testNamePattern #4119

Closed

Conversation

segrey
Copy link
Contributor

@segrey segrey commented Sep 12, 2023

Description

This is another attempt to fix #4103.
Pro: the fixed version of getTaskFullName is restored: no leading space makes #4045 fixed again.
Con: taskFullName.match(namePattern) is called twice.

@sheremet-va @Dunqing

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@stackblitz
Copy link

stackblitz bot commented Sep 12, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Sep 12, 2023

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit c93f74b
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/6509e388f25d940008ca3026

@segrey
Copy link
Contributor Author

segrey commented Sep 12, 2023

I'm not entirely sure if it's better than the current solution (#4119). Just wanted to share with you to discuss.

@sheremet-va
Copy link
Member

I think this change is ok. If you can fix the failing CI, we can merge it.

@Dunqing
Copy link
Member

Dunqing commented Sep 19, 2023

It looks like it just needs a lint fix, do you have time to look here and finally merge it before releasing the next version

@segrey segrey force-pushed the testNamePattern-leading-space-2 branch from 547d36b to c93f74b Compare September 19, 2023 18:08
@segrey
Copy link
Contributor Author

segrey commented Sep 19, 2023

@sheremet-va @Dunqing Done, please take a look.

@segrey
Copy link
Contributor Author

segrey commented Oct 18, 2023

@sheremet-va Could you please take a look? Thanks!

@sheremet-va
Copy link
Member

I don't see the need for this PR personally. WebStorm should be fixed already, and the tool should change the behavior in the next version to not rely on the leading space.

Vitest eventually will remove the space (probably in v1?)

@segrey
Copy link
Contributor Author

segrey commented Oct 18, 2023

Yes, WebStorm is fully covered: (a) now it passes testNamePattern with optional leading space and (b) thanks to releasing Vitest 0.34.5 with the restored previous behavior. I can only think of this PR as a smooth transition for other clients also passing testNamePattern with a leading space now. It might be a surprise for them when Vitest v1 is released. Otherwise, this PR adds no value indeed.

@sheremet-va
Copy link
Member

I can only think of this PR as a smooth transition for other clients also passing testNamePattern with a leading space now. It might be a surprise for them when Vitest v1 is released. Otherwise, this PR adds no value indeed.

I would expect other clients to migrate as it is a breaking change like any other.

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.

Changed testNamePattern behavior breaks IntelliJ / WebStorm (vitest 0.34.4)
3 participants