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

added --dry-run option #1028

Merged
merged 6 commits into from Aug 22, 2013
Merged

added --dry-run option #1028

merged 6 commits into from Aug 22, 2013

Conversation

schnittchen
Copy link
Contributor

@schnittchen schnittchen commented Jul 29, 2013

This adds --dry-run support as requested in #1022.

I would need some help as to where to test this feature...

@coveralls
Copy link

coveralls commented Jul 29, 2013

Coverage Status

Coverage increased (+0%) when pulling 9582260 on schnittchen:dry-run into a04fb17 on rspec:master.

@@ -107,7 +107,7 @@ def run(example_group_instance, reporter)
start(reporter)

begin
unless pending
unless pending or RSpec.configuration.dry_run
Copy link
Member

@penelopezone penelopezone Jul 29, 2013

Choose a reason for hiding this comment

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

This is very strongly opinionated so feel free to disagree with me better, but as conditionals get more complex I like turn them into ifs instead of unlesses. What do you think?

Copy link
Contributor Author

@schnittchen schnittchen Jul 29, 2013

Choose a reason for hiding this comment

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

I am totally ok with changing this to "if". I realize that coming from a mathematics background, DeMorgan's rules are pretty hard-wired into my brain...

Copy link
Contributor Author

@schnittchen schnittchen Jul 29, 2013

Choose a reason for hiding this comment

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

Or extract the condition out into a will_execute? method?

Copy link
Member

@JonRowe JonRowe Jul 30, 2013

Choose a reason for hiding this comment

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

Normally I would agree with you @samphippen, especially if there's an else but I try to stick to the cleaner version and

unless pending or RSpec.configuration.dry_run

reads better than

if !(pending or RSpec.configuration.dry_run)
if !pending and !RSpec.configuration.dry_run

@JonRowe
Copy link
Member

JonRowe commented Jul 30, 2013

You can add specs to this in spec/rspec/core/configuration_options_spec.rb for checking the option is parsed correctly, and spec/rspec/core/example_spec.rb to check that an example group won't run if dry run is enabled.

You will also need to add a .feature to document the feature and will serve as an integration test to assert that everything works together.

@myronmarston
Copy link
Member

myronmarston commented Jul 30, 2013

I think this implementation will still run before(:all) and after(:all) hooks, which it shouldn't.

@schnittchen
Copy link
Contributor Author

schnittchen commented Jul 30, 2013

Correct. Argh. More research.

@schnittchen
Copy link
Contributor Author

schnittchen commented Jul 30, 2013

I think I got it. Should probably move tests around after I realized before/after(:all) block execution is a group thing. And please nitpick if you see things you don't like.


Scenario: Using --dry-run
When I run `rspec . --dry-run`
Then the examples should all pass
Copy link
Member

@JonRowe JonRowe Jul 30, 2013

Choose a reason for hiding this comment

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

I think it would be better to assert on the output here, to give an example of what to expect, and I'm not sure we should be considering the tests as "passing" here. They're closer to being pending.

Copy link
Contributor Author

@schnittchen schnittchen Jul 31, 2013

Choose a reason for hiding this comment

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

I would rather write something like "Then it looks like the tests are all passing" with the same assertion underneath.

Copy link
Member

@JonRowe JonRowe Jul 31, 2013

Choose a reason for hiding this comment

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

The test's don't pass though, they don't even run, so to describe it as such is misleading in my opinion

Copy link
Contributor Author

@schnittchen schnittchen Jul 31, 2013

Choose a reason for hiding this comment

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

I see. Would "Then it looks like the tests are all passing" solve that? Do you have a better suggestion?

Copy link
Member

@JonRowe JonRowe Jul 31, 2013

Choose a reason for hiding this comment

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

Actually showing the output you expect, similar to how pending does it /features/pending/pending_examples.feature#L17

Edit (is what I suggest :) )

Copy link
Contributor Author

@schnittchen schnittchen Jul 31, 2013

Choose a reason for hiding this comment

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

Updated.

@coveralls
Copy link

coveralls commented Aug 1, 2013

Coverage Status

Coverage increased (+0%) when pulling cdd06b0 on schnittchen:dry-run into a04fb17 on rspec:master.


Scenario: Using --dry-run
When I run `rspec . --dry-run`
Then the output should contain "5 examples, 0 failures"
Copy link
Member

@JonRowe JonRowe Aug 1, 2013

Choose a reason for hiding this comment

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

Nice work on the compacting, although I'd be tempted to use one liners for the hooks; Also this should now be "1 example"

Copy link
Contributor Author

@schnittchen schnittchen Aug 1, 2013

Choose a reason for hiding this comment

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

Oh my. Blanked on that. Will fix it tonight.

@coveralls
Copy link

coveralls commented Aug 1, 2013

Coverage Status

Coverage increased (+0%) when pulling 8ac274e on schnittchen:dry-run into 505939b on rspec:master.

@JonRowe
Copy link
Member

JonRowe commented Aug 2, 2013

LGTM, but we'll wait until @myronmarston returns to merge this I think

@schnittchen
Copy link
Contributor Author

schnittchen commented Aug 2, 2013

Cool! Let me know if you want me to squash the commits.

myronmarston added a commit that referenced this issue Aug 22, 2013
@myronmarston myronmarston merged commit 32ec674 into rspec:master Aug 22, 2013
1 check passed
@myronmarston
Copy link
Member

myronmarston commented Aug 22, 2013

Thanks for your contribution, @schnittchen!

I merged it, and then built off of this to add a few further improvements: #1056.

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