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

Add ability to set test_dir using an environment variable #35

Merged
merged 4 commits into from
Apr 11, 2016

Conversation

jnraine
Copy link
Contributor

@jnraine jnraine commented Apr 11, 2016

Prior to this commit, test_dir was extracted from the test file pattern using a regex. For example, using the default RSpec test file pattern of spec/**{,/*/**}/*_spec.rb, test_dir would be extracted in one of two ways:

"spec/**{,/*/**}/*_spec.rb".split('/').first # => "spec"
"spec/**{,/*/**}/*_spec.rb".gsub(/^(.*?)\//).first # => "spec/"

The end result is effectively the same and works great for this case. However, when using a more complex test file pattern this method of extraction fails to provide the desired result. For example, say we had multiple spec directories under different trees:

"{spec,engines/**/spec}/**{,/*/**}/*_spec.rb".split('/').first # => "{spec,engines"
"{spec,engines/**/spec}/**{,/*/**}/*_spec.rb".gsub(/^(.*?)\//).first # => "{spec,engines/"

In this case, the extracted test_dir is not usuable.

This commit modifies the code to use the value specified in ENV["KNAPSACK_DEFAULT_TEST_DIR"], otherwise to extract the test_dir in the normal way.

Note: We could attempt to make extraction more intelligent (and thereby more complex) but allowing one to specify it directly may be the simplest working solution for everyone.

Prior to this commit, `test_dir` was extracted from the test file pattern using a regex. For example, using the default RSpec test file pattern of `spec/**{,/*/**}/*_spec.rb`, `test_dir` would be extracted in one of two ways:

```ruby
"spec/**{,/*/**}/*_spec.rb".split('/').first # => "spec"
"spec/**{,/*/**}/*_spec.rb".gsub(/^(.*?)\//).first # => "spec/"
```

The end result is effectively the same and works great for this case. However, when using a more complex test file pattern this method of extraction fails to provide the desired result. For example, say we had multiple spec directories under different trees:

```ruby
"{spec,engines/**/spec}/**{,/*/**}/*_spec.rb".split('/').first # => "{spec,engines"
"{spec,engines/**/spec}/**{,/*/**}/*_spec.rb".gsub(/^(.*?)\//).first # => "{spec,engines/"
```

In this case, the extracted `test_dir` is not usuable.

This commit modifies the code to use the value specified in `ENV["KNAPSACK_DEFAULT_TEST_DIR"]`, otherwise to extract the `test_dir` in the normal way.

_Note: We could attempt to make extraction more intelligent (and thereby more complex) but allowing one to specify it directly may be the simplest working solution for everyone._
@ArturT
Copy link
Member

ArturT commented Apr 11, 2016

Could you add example in readme in FAQ section. https://github.com/ArturT/knapsack#faq

When you provide KNAPSACK_DEFAULT_TEST_DIR then you will run tests only for this directory or you will have to specify twice rake knapsack:rspec with two different KNAPSACK_DEFAULT_TEST_DIR to run your whole test suite.

How are you using this in your case? Good example in read me would be great.

@jnraine
Copy link
Contributor Author

jnraine commented Apr 11, 2016

Certainly! We're trying to setup Knapsack for our app on BuildKite. We use RSpec and in addition to the normal spec/ directory, we also have several engine spec directories—e.g., engines/my_engine/spec. Prior to BuildKite and Knapsack, we ran our entire test suite using a single command:

$ bundle exec rspec spec engines/**/spec

This takes advantage of the default spec directory for RSpec—spec/—allowing require 'spec_helper' to work correctly because $: is modified to include spec/ while also running all specs from our engines.

Initially, I attempted to pass these same arguments to the Knapsack rake task, but the resulting command gave unexpected results (with good reason):

$ bundle exec rake "knapsack:rspec[spec engines/**/spec]"
# `Knapsack::Runner::RSpecRunner` executes:
$ bundle exec rspec spec engines/**/spec --default-path spec -- spec/models/account_spec.rb spec/models/user_spec.rb engines/my_engine/spec/models/invoice_spec.rb engines/my_other_engine/spec/models/document_spec.rb

This stops Knapsack from being able to control what specs are run in each node because we've told RSpec to run the directories directly.

Specifying a more complex test file pattern and omitting the arguments half-fixes the issue, giving control of which specs are run back to Knapsack, but results in an unusable test directory:

$ bundle exec rake "knapsack:rspec[spec engines/**/spec]"
# `Knapsack::Runner::RSpecRunner` executes:
$ bundle exec rspec --tag js --default-path {spec,engines/ -- spec/models/account_spec.rb spec/models/user_spec.rb engines/my_engine/spec/models/invoice_spec.rb engines/my_other_engine/spec/models/document_spec.rb

This is what led me to believe the ability to specify a test directory would be useful. Let me know if you have any other questions about this. I'll add an example to the FAQ.

@jnraine
Copy link
Contributor Author

jnraine commented Apr 11, 2016

Thinking about the environment variable further, I wonder if a better name is simply KNAPSACK_TEST_DIR. The "default" portion of the name seems superfluous.

@ArturT
Copy link
Member

ArturT commented Apr 11, 2016

@jnraine Thanks for explanation. Could you add example case with using KNAPSACK_TEST_DIR in .travis.yml. You can add engine_spec_examples directory for other specs or something like that. Thanks!

@jnraine
Copy link
Contributor Author

jnraine commented Apr 11, 2016

@ArturT done!

@ArturT ArturT merged commit 3c96b74 into KnapsackPro:master Apr 11, 2016
@ArturT
Copy link
Member

ArturT commented Apr 11, 2016

@jnraine Thanks for great work! I've just released knapsack v1.7.0.

BTW If your test suite is big then you may like to try http://knapsackpro.com and get free API key.

@jnraine
Copy link
Contributor Author

jnraine commented Apr 11, 2016

Nice. Thanks for the fast merge and release @ArturT!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants