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

feat: support .each suite execution by regex #214

Merged
merged 21 commits into from
Feb 19, 2024

Conversation

keroxp
Copy link
Contributor

@keroxp keroxp commented Jan 19, 2024

Support an execution of .each describe/test suite that includes % placeholders. vitest --testNamePattern opts doesn't recognize pattern with% placeholders. So we need to transform the given suite name by replacing % to .+ regexp.

The base branch of this PR is #213.

@keroxp keroxp marked this pull request as ready for review January 19, 2024 06:21
@keroxp keroxp marked this pull request as draft January 19, 2024 10:53
@keroxp keroxp changed the title feat: support .each suite execution by regex [WIP] feat: support .each suite execution by regex Jan 19, 2024
@keroxp keroxp changed the title [WIP] feat: support .each suite execution by regex feat: support .each suite execution by regex Jan 25, 2024
@keroxp keroxp marked this pull request as ready for review January 25, 2024 07:18
@MilanKovacic
Copy link
Collaborator

MilanKovacic commented Feb 15, 2024

Fixes #7.

@keroxp
Copy link
Contributor Author

keroxp commented Feb 16, 2024

Additionally, Incomplete edge cases still remain. Similar describe/test cases will be matched by the same transformed regular expression. Like:

(1) test.each([...])("my test %s)(() => {})
(2) test.each([...])("my test will be %s")(()=>{})

(2) is also executed by executing (1) and got no result, showing error, because (1) goes to my test .+?.

@sheremet-va
Copy link
Member

Thank you for your contribution! Can you resolve conflicts, when you have the time, please? 🙏🏻

@keroxp
Copy link
Contributor Author

keroxp commented Feb 19, 2024

Ready for review.

Copy link
Collaborator

@ffMathy ffMathy left a comment

Choose a reason for hiding this comment

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

LGTM! I love this. Initially I was skeptical of the use of regex, but after seeing the implementation, it should be stable.

@ffMathy
Copy link
Collaborator

ffMathy commented Feb 19, 2024

@keroxp, @sheremet-va wants to know if we can merge this?

@keroxp
Copy link
Contributor Author

keroxp commented Feb 19, 2024

@ffMathy I'm OK

@sheremet-va sheremet-va merged commit 4ceb41c into vitest-dev:main Feb 19, 2024
1 check passed
@ffMathy
Copy link
Collaborator

ffMathy commented Feb 22, 2024

This introduces a bug. When there are more describes with the same name, you can't run the tests in the last describe.

Example repro:

import { describe, expect, it } from 'vitest'
import { add } from '../src/add'

describe('duplicate describe', () => {
  it('test 1', () => {
    expect(add(1, 1)).toBe(2)
  })
})

describe('duplicate describe', () => {
  it('test 2', () => {
    expect(add(1, 1)).toBe(2)
  })
})

With the above, "test 2" can't be run and will give an error. This worked before.

@ffMathy
Copy link
Collaborator

ffMathy commented Feb 22, 2024

cc @keroxp. Got an idea for a fix? I tried, but can't understand the code.

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.

4 participants