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

Make glob requirements clearer #54

Closed
wants to merge 1 commit into from
Closed

Conversation

niceking
Copy link
Collaborator

@niceking niceking commented May 6, 2024

Description

As raised by @TJC in #52, the phrasing of what we expect the glob to be could be clearer

| `BUILDKITE_SPLITTER_TEST_FILE_PATTERN` | `spec/**/*_spec.rb` | Optional, glob pattern for discovering test files that need to be executed. </br> *It accepts pattern syntax supported by [zzglob](https://github.com/DrJosh9000/zzglob?tab=readme-ov-file#pattern-syntax) library*. |
| `BUILDKITE_SPLITTER_TEST_FILE_EXCLUDE_PATTERN` | - | Optional, glob pattern to use for excluding test files or directory. </br> *It accepts pattern syntax supported by [zzglob](https://github.com/DrJosh9000/zzglob?tab=readme-ov-file#pattern-syntax) library.* |
| `BUILDKITE_SPLITTER_TEST_FILE_PATTERN` | `spec/**/*_spec.rb` | Optional, file glob pattern for discovering test files that need to be executed. </br> *It accepts pattern syntax supported by [zzglob](https://github.com/DrJosh9000/zzglob?tab=readme-ov-file#pattern-syntax) library*. |
| `BUILDKITE_SPLITTER_TEST_FILE_EXCLUDE_PATTERN` | - | Optional, file glob pattern to use for excluding test files or directory. </br> *It accepts pattern syntax supported by [zzglob](https://github.com/DrJosh9000/zzglob?tab=readme-ov-file#pattern-syntax) library.* |
Copy link

Choose a reason for hiding this comment

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

It says for excluding test files or directory. -- so can I use the pattern to exclude a whole directory tree? You suggested in #52 that it had to match files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @TJC, yes you can use BUILDKITE_SPLITTER_TEST_FILE_EXCLUDE_PATTERN to exclude a whole directory, providing you use the correct globing pattern syntax. The issue you were having in #52 seems to be caused by the issue in the identifier that you raised in #55.

Can you confirm that the exclude pattern is now working on your side?

Copy link

Choose a reason for hiding this comment

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

I was referring to the comment in #52 (comment) which said "We expect the exclude pattern to take the form of a file glob, not a directory path as you're probably used to doing with Rspec!"

The description you're changing in the PR now says "file glob pattern to use for excluding test files or directory."
That seems to imply you could pass a glob that matched a directory.

So perhaps you can see where this documentation isn't actually that clear about whether it accepts a directory, or if you actually need to be creating a glob that matches all files within a directory. Based on our discussions I think the answer is "a glob that matches all files within a directory", but to a fresh reader who comes to this project, I don't think that would be clear.

Copy link

Choose a reason for hiding this comment

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

There's also been #58 merged since this PR was opened, which I think has changed how it works?

@wooly
Copy link
Contributor

wooly commented May 21, 2024

I think this has been rendered unnecessary by the change that @DrJosh9000 made to the zzglob library and subsequent 0.4.0 release of the splitter, so I'm going to close this.

@niceking feel free to re-open though if you think there's more we can do.

@wooly wooly closed this May 21, 2024
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.

None yet

5 participants