search

buildkite / test-engine-client

Buildkite Test Engine Client (bktec) is an open source tool to orchestrate your test suites. It uses your Buildkite Test Engine suite data to intelligently partition and parallelise your tests.
MIT License
9 stars 1 forks source link

Exclude `node_modules` all the time #207

Closed nprizal closed 3 weeks ago

nprizal commented 4 weeks ago

Currently, we set the default of BUILDKITE_TEST_ENGINE_TEST_EXCLUDE_PATTERN to be **/node_modules to exclude files within node_modules directory. If customers configure the BUILDKITE_TEST_ENGINE_TEST_EXCLUDE_PATTERN to their needs, files inside the node_modules can accidentally get picked up by the client. We need to exclude files inside node_modules all the time.

wooly commented 4 weeks ago

I'm not sure I 100% agree with this change. For me, we should either use the default value if an override isn't provided, or do what the user says they want to do.

Did someone raise this as an issue?

nprizal commented 4 weeks ago

I'm not sure I 100% agree with this change. For me, we should either use the default value if an override isn't provided, or do what the user says they want to do.

Did someone raise this as an issue?

This is to avoid accidentally running a test inside node_modules when customers configure the exclude patterns. Without this, the customer will have to write a complicated glob pattern to exclude node_modules plus other patterns. Customer might not aware that they have to also exclude node_modules explicitly if they want to configure the exclude patterns.

Do you think there will be a case where a customer wants to run a test inside node_modules, I doubt so?

wooly commented 3 weeks ago

This is to avoid accidentally running a test inside node_modules when customers configure the exclude patterns. Without this, the customer will have to write a complicated glob pattern to exclude node_modules plus other patterns. Customer might not aware that they have to also exclude node_modules explicitly if they want to configure the exclude patterns.

Do you think there will be a case where a customer wants to run a test inside node_modules, I doubt so?

I doubt there will be either, it's just the principle of 'We provide all of the behaviour through the default or you provide all of the behaviour by overriding that default' feels a bit nicer than 'We provide all of the behaviour through the default or you provide some of the behaviour with us adding to it if you override it'. It feels like this is a downside of using the environment variables as a configuration pattern. File-based formats like YAML or JSON or TOML would allow you to specify an array of regex values to exclude rather than having to craft a single regex to handle it all. We don't have that though, so it's a bit moot.

Happy for this to be non-blocking and just something to keep in mind.

nprizal commented 3 weeks ago

Thanks for putting your thoughts, everyone. I'll go forward with these changes. We can update this if customers find this harder to use, or when we switch to using a configuration file that will allow us to have more flexibility in configuring the pattern.