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

Mark pending blocks as failed if they succeed. #1267

Merged
merged 1 commit into from Feb 10, 2014
Merged

Conversation

xaviershay
Copy link
Member

@xaviershay xaviershay commented Jan 26, 2014

DO NOT MERGE: This needs final review.

See #1208 for background.

@grddev @myronmarston @JonRowe @markijbema

@xaviershay
Copy link
Member Author

xaviershay commented Jan 26, 2014

Spec failure is due to pending block not currently supporting message expectations (coming from mocks specs).

That is going to require an rspec-mocks change, let me see if that is going to be a lot of work.

@xaviershay
Copy link
Member Author

xaviershay commented Jan 26, 2014

Pushed up a commit that addresses the issue, though build won't be green until rspec/rspec-mocks#544 is merged - chicken and egg problem. It would be possible with a few extra PRs to merge this in a way that keeps everything green but I don't think it's worth it.

I'm not sure whether I like this solution overall :/

@markijbema
Copy link

markijbema commented Jan 26, 2014

Just to give my feedback as a user:

I actually never knew that when you used pending inside a test it had different behaviour than when you made the whole test (it) pending. Surely it's documented somewhere, but it's the sortof documentation I would look up when I wanted to know how something works, but in this case, pending already worked for me, so I never bothered to read the docs on it. So I think that improving the consistency is a clear improvement.

I tend to use pending for a bunch of different scenarios. Without body, when I just write out some tests I have to write later. I make an existing test pending when it causes trouble, and isn't worthwhile to fix now (ie. an acceptance test which fails intermittently, and is testing it isn't very critical right now), this is basically a 'fixme'.

Personally, I never used the pending case supported here, and must admit to not totally getting the usecase of this. Is this to do red-green-refactor without getting an actual red?

Last thought is that I don't really like 'xit'. It feels really hackish. Otoh, maybe my regular use is rather hackish, so that is okay.

@myronmarston
Copy link
Member

myronmarston commented Jan 27, 2014

@markijbema -- thanks for engaging with us on this! It's always great to get feedback from rspec users about changes we're making.

Personally, I never used the pending case supported here, and must admit to not totally getting the usecase of this. Is this to do red-green-refactor without getting an actual red?

I've used this form of pending on many occasions. Generally, I use it when I have a spec that I've written that, due to a bug in something I'm not currently working on (e.g. a gem the project depends on, or a bit of the project that a coworker is responsible for), there's no way to get passing, but that I expect will pass in the future after I update to a newer version of a gem with a bugfix or my coworker updates their bit of the project. In situations like these, I want to keep the spec I've written, but make it pending since I want my build to stay green so that we can keep making progress. I use the block form of pending so that RSpec informs me as soon the spec starts to pass, so I know I can make it no longer pending. (This could happen during a later bundle update, for example).

Last thought is that I don't really like 'xit'. It feels really hackish. Otoh, maybe my regular use is rather hackish, so that is okay.

xit isn't really intended to ever by committed (at least, not to commits you plan to keep as-is). It's intended as a quick, easy way to skip a spec: just prefix your it with an x. It's optimized for convenience over readability. I don't recommend using it for pages where you plan to push commits with some specs skipped. Before now I would have recommended using pending (as it is more readable/permanent) but now the semantics are changing a bit. Maybe we should add a skip method? That is more readable/obvious what it means/does. It would be the equivalent of ExampleGroup.pending from 2.x, and we can say that xit is a shortcut for skip.

@xaviershay -- I still owe you a full review of this. Still planning on doing that, but wanted to respond to @markijbema's feedback for now.

@JonRowe
Copy link
Member

JonRowe commented Jan 27, 2014

I tend to use pending for a bunch of different scenarios. Without body, when I just write out some tests I have to write later. I make an existing test pending when it causes trouble, and isn't worthwhile to fix now (ie. an acceptance test which fails intermittently, and is testing it isn't very critical right now), this is basically a 'fixme'.

Writing it "will do something later" without a block will still be skipped as per usual.

define_method(name) do |*all_args, &block|
desc, *args = *all_args
options = Metadata.build_hash_from(args)
options.update(:pending => RSpec::Core::Pending::NOT_YET_IMPLEMENTED) unless block
options.update(:skip => RSpec::Core::Pending::NOT_YET_IMPLEMENTED) unless block
Copy link
Member

@JonRowe JonRowe Jan 27, 2014

Choose a reason for hiding this comment

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

As we now have essentially two behaviours, "Pending" and "Skip" perhaps we should name these as such?

@JonRowe
Copy link
Member

JonRowe commented Jan 27, 2014

This LGTM apart from the fact that I wonder wether we should split skip and pending more so? Also the stuff you've added to eventually allow the extension the behaviour of pending, we've not really talked about how that'd work (or even why'd you want it), and there doesn't seem to be much in the way of documentation/specs for it?

@markijbema
Copy link

markijbema commented Jan 27, 2014

@myronmarston I'm only using xit for situations where I shouldn't commit it. Sometimes I don't have the time to fix it properly though, and I do. This typically involves an integration test which fails because it is written poorly, and fails even though conceptually the functionality works. Sometimes the logic is unfortunately so complex that I decide to postpone this post my pull-request, and fix it in a seperate one (for instance, one of our helpers we use in capybara tests has to be rewritten because there is a critical flaw in it; rewriting it in my own pull-request also feels wrong, because it is totally unrelated).

I like the suggestion of 'skip' though. I think that gives a nice semantic difference between skip and pending.

Also, I really like that the rspec team dares to drop backwards compatibility to improve the product. Keep up the great work :)

@@ -77,6 +77,22 @@ Feature: pending examples
And the output should contain "Expected pending 'something else getting finished' to fail. No Error was raised."
And the output should contain "pending_with_passing_block_spec.rb:3"

Scenario: pending any arbitrary reason, with a top-level block that passes
Copy link
Member

@myronmarston myronmarston Feb 1, 2014

Choose a reason for hiding this comment

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

I'm not sure what "a top-level block" means in this context. I tend to think of "top-level" as being in the context of main but that's not the case here. How about "Using pending to define an example that is currently passing"?

Copy link
Member Author

@xaviershay xaviershay Feb 1, 2014

Choose a reason for hiding this comment

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

I wasn't happy with this description either.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 1, 2014

Thanks for all the comments! Pretty sure I know how to address most of them, working on it today.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 1, 2014

See new commits, I think they address everything.

I just rebased this and @myronmarston's new alias_example_group_to feature is failing with these changes, so need to investigate that some more.

#
# @see RSpec::Core::Pending#pending
def pending(*all_args, &block)
desc, *args = *all_args
Copy link
Member

@myronmarston myronmarston Feb 1, 2014

Choose a reason for hiding this comment

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

Why not just define this as:

def pending(desc, *args, &block)
  # ...
end

...rather than pending(*all_args) and then split it on the first line?

Copy link
Member Author

@xaviershay xaviershay Feb 1, 2014

Choose a reason for hiding this comment

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

needs a default of nil, but yeah that works.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 1, 2014

Build failures are the same expected RSpec mock errors.

@@ -888,26 +888,55 @@ def define_and_run_group(define_outer_example = false)
end
end

%w[pending xit xspecify xexample].each do |method_name|
describe "::pending" do
Copy link
Member

@myronmarston myronmarston Feb 1, 2014

Choose a reason for hiding this comment

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

I've mentioned this elsewhere, but I'm not a fan of using :: for message sends. I know you're being consistent, but my preference is to change all the places we work in to use the more standard . for message sends. Mind updating it?

@xaviershay
Copy link
Member Author

xaviershay commented Feb 2, 2014

See last 4 commits. The implementation of new pending behaviour feels ... uncomfortable, possibly just because this code felt uncomfortable to start with and I haven't done any work it in before. I don't totally have all the begin/rescue blocks and different states in my head...

@xaviershay
Copy link
Member Author

xaviershay commented Feb 2, 2014

Turns out changing the behaviour of pending inside an example also accidentally fixed the rspec mocks problem! I added some explicit spec coverage anyways, and removed the pending executor stuff.

I think all feedback has been addressed now. There is the open issue of whether we want to provide a different interface to formatters, since that won't be a backwards incompatible change I don't think it needs to be included here.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

pending.rb and example.rb need another review, I changed them pretty significantly. The logic is a bit clearer now.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

urgh actually hold off, still a few more outstanding issues.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

This is still a bit of mess when interacting with mocks, but for the most part you just get specs marked as pending.

See 41ab9a7#diff-3bac19fc9abdbdd90aaec18fabf14a95R211 for my current understanding of the problem.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

We could also just ditch this entire "speculatively run pending blocks" feature. It's not that useful and certainly complicates things a lot.

@myronmarston
Copy link
Member

myronmarston commented Feb 9, 2014

Does anyone have a dev environment already set up that lets them easily regenerated all the HTML formatter fixtures? I'm not feeling motivated to install all the required rubies... Need to run the following:

I think I can do this. I'll take a stab at it later tonight (I'm only online for a couple minutes right now).

We could also just ditch this entire "speculatively run pending blocks" feature. It's not that useful and certainly complicates things a lot.

I'm personally a fan of this feature and it's never been a maintenance burden before. It also seems like the kind of thing that would be hard to achieve in an extension gem using public APIs. I can understand your frustration, though...I'll try to take a look at things later tonight to see where things stand.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

I'm mostly worried about shipping an inconsistent feature. Although it's inconsistent now...

Actually, if we remove the pending-with-a-block-in-an-example feature, I think the problem goes away. You can still use skip with :if to conditionally evaluate blocks. You still get execute and fail on pass behaviour from either normal pending keyword or pending at example group scope. Thoughts?

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

Semantically, saying "this part of the spec is broken now, and should work in the future, but the rest of the spec still works fine and is valuable" seems unlikely and confusing to me. Instead, marking it pending halfway through (without a block) seems a lot clearer and easy to reason about.

Even in the case of an acceptance spec, executing a pending block half-way through the spec is problematic since it puts you in an unknown state.

To put it another way, I think the high-level functionality that used to be provide by pending-with-a-block ("let me know when this passes") is useful, but the old API for it is no good. By keeping the functionality but with the new API, and removing the old API, we get a much better product.

@myronmarston
Copy link
Member

myronmarston commented Feb 9, 2014

Ah, I misunderstood what you were talking about. I thought you were talking about the feature as a whole, and not simply the "pending block from within an example" aspect (which was the only API to the feature in 2.x). Now that I get what you're saying, I think I agree: it doesn't seem needed or useful, and the new API seems much better.

Actually, there is one case where it might still be useful: for code that is being written against both the RSpec 2.x API and RSpec 3.x API. Pending block within an example is the only API for this in RSpec 2.x, and if we remove that, there is no common API for both versions people can use. That's probably not a big deal though: I can't think of any rspec extension gems that use this (as it's generally something in a specific end-user spec, not in an extension gem). It might affect Sequel, though, as I know it's suite is meant to work on RSpec 1.x, 2.x and 3.x (see jeremyevans/sequel@101cc3a).

I don't think that's a strong enough argument for it, though....so my vote is to axe it.

@markijbema
Copy link

markijbema commented Feb 9, 2014

Noticed that you have to do some effort to generate those fixtures locally. Is this something which needs to be done a lot? If so, you could consider leveraging Travis to generate them for you.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

I thought you were talking about the feature as a whole

Initially I was, but I changed :)

Pending block within an example is the only API for this in RSpec 2.x, and if we remove that, there is no common API for both versions people can use.

If we remove it completely, then pending { whatever } will still "work", it will mark the example as pending without running the block. That doesn't seem terrible. (In 2.9 this case will deprecate and recommend a change to skip or no block.)

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

@markijbema This would be the second time I've had to do it. They change pretty rarely. I really just need to get my dev environment set up properly...

@grddev
Copy link

grddev commented Feb 9, 2014

As I said earlier, my only strong opinion is that there should be a forward-compatible transition from RSpec 2.x, that:

  • Has the new behaviour
  • Will continue to work in 3.x
  • Does not produce any deprecation warnings in 2.99

Cutting the pending block feature seems to remove this possibility, or have I misunderstood something?

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

@grddev you can either switch to skip with a block, or just remove the block from your pending call. 2.9 will suggest both of these options.

@markijbema
Copy link

markijbema commented Feb 9, 2014

@grddev I understand what you want, and I agree that it is preferable, but there isn't really a way if we want to call this feature 'pending' in 3, right? Is there a way rspec can differentiate between a pending with block which was meant to be run, and fail if succeed, and a block which was meant not to be run? The only solution I see which would be a little closer is allow something like:

pending "foo", run: true do
end

which shows that it should be run in 2.99, and have the run param be optional in 3.

Would it be an idea to make the pending inside the block deprecated from version 3 onward? I don't think it has much more value above this syntax, right? I think that would solve the transition, since then you could transition in 2.99, and transition in 3 again. It's not perfect though. I don't see how you could solve this with one version though. For a gem I maintain we did some sequential upgrades to work around problems like this (reappropriating existing methods/names). You could also do it with 2.98 and 2.99 of course, but it feels a bit like a kludge.

@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

HTML fixtures need re-gen ... am hoping to be able to rebase this on top of #1306 to solve that problem.

@myronmarston
Copy link
Member

myronmarston commented Feb 9, 2014

If we remove it completely, then pending { whatever } will still "work", it will mark the example as pending without running the block. That doesn't seem terrible.

Might be worth warning and/or raising an error if a block is given to pending since it could be a common mistake from people not ware of the changes who are used to 2.x behavior, but I agree, either way, not terrible.

* Execute pending examples and mark as failed if they succeed.
* Removed pending with a block.
* Introduce `skip` method and metadata for old pending behaviour.

This is a backwards-incompatible change.

Implements #1208.
@xaviershay
Copy link
Member Author

xaviershay commented Feb 9, 2014

I'm done making changes to this, please give it a final once over.

Might be worth warning and/or raising an error if a block is given to pending.

We're already providing a warning in 2.99, I don't think it's worth the code here. In the common case you'll get flagged anyway, since the block won't execute and the example will pass, causing a failure.

an example is skipped using xexample
# Temporarily skipped with xexample
# ./temporarily_skipped_spec.rb:8
"""
Copy link
Member

@myronmarston myronmarston Feb 10, 2014

Choose a reason for hiding this comment

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

There's a 5th way: tagging an example with :skip => "reason why". Not a merge blocker.

@myronmarston
Copy link
Member

myronmarston commented Feb 10, 2014

Thanks for working so hard on this, @xaviershay. I left a few more comments, but any of them that are worth addressing can be addressed in a separate PR. I'm 100% OK with this being merged as is.

@markijbema
Copy link

markijbema commented Feb 10, 2014

Thanks!

@xaviershay
Copy link
Member Author

xaviershay commented Feb 10, 2014

Merging. Will investigate potential bugs around the set_exception, but I'd prefer to get this in beta2 to get some real usage out of this.

xaviershay added a commit that referenced this issue Feb 10, 2014
Mark pending blocks as failed if they succeed.
@xaviershay xaviershay merged commit facc88c into master Feb 10, 2014
@xaviershay xaviershay mentioned this pull request Feb 17, 2014
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