Feature request: shortcut for pending-block within it #1208

Closed
grddev opened this Issue Dec 6, 2013 · 13 comments

Comments

Projects
None yet
5 participants
@grddev

grddev commented Dec 6, 2013

Using pending inside an it block is a useful feature for ensuring that tests fail before touching the code, but there seems to be no shortcut for that kind of pending. That is, I would like to have an alternative to

pending 'returns truthy' do
  expect(true).to be(true)
end

or

it 'returns truthy' do
  pending
  expect(true).to be(true)
end

that works exactly like:

it 'returns truthy' do
  pending do
    expect(true).to be(true)
  end
end

The difference being that in the latter case, the code is executed and a failure is reported unless the inner test actually fails. Perhaps the name pending! could be used in both of the above cases to signify the case where the tests should still be executed.

The main motivation would to allow a work flow where a forced "red before green" workflow can be enforced by separating the commits, while not having to fiddle with indentation in the subsequent commit test cases.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 6, 2013

Member

I think I like this idea. I've wanted the same thing at various points. Currently pending 'doc string' is just an alias for it 'doc string', :pending => true. xit is basically the same. In RSpec 3.0, we could retain xit as a way to make an example pending w/o running it (and :pending => true or just :pending would do the same). And then pending could run the example as it looks identical to the block form in the it.

For now I think you can get this on your own pretty simply:

module RunPending
  def pending(*args, &block)
    it(*args) { pending(&block) }
  end
end

RSpec.configure do |c|
  c.extend RunPending
end

If we're going to make this change, 3.0 is the time to do it as it's a breaking change. We'll need to issue a deprecation warning in 2.99.

Member

myronmarston commented Dec 6, 2013

I think I like this idea. I've wanted the same thing at various points. Currently pending 'doc string' is just an alias for it 'doc string', :pending => true. xit is basically the same. In RSpec 3.0, we could retain xit as a way to make an example pending w/o running it (and :pending => true or just :pending would do the same). And then pending could run the example as it looks identical to the block form in the it.

For now I think you can get this on your own pretty simply:

module RunPending
  def pending(*args, &block)
    it(*args) { pending(&block) }
  end
end

RSpec.configure do |c|
  c.extend RunPending
end

If we're going to make this change, 3.0 is the time to do it as it's a breaking change. We'll need to issue a deprecation warning in 2.99.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Dec 12, 2013

Member

Let's do it, @grddev do you fancy taking a stab at this?

Member

JonRowe commented Dec 12, 2013

Let's do it, @grddev do you fancy taking a stab at this?

@grddev

This comment has been minimized.

Show comment
Hide comment
@grddev

grddev Dec 15, 2013

I could probably take a stab at it, but from my point of view it seems a bit surprising to have a pending block that behaves differently to it "...", :pending and it "...", :pending => true. To me it would seem more natural to clearly split the two concepts

  • bypass: ignore the tests completely (xit and friends, and current pending)
  • pending: specify future desired behavior (current it with pending block)

I think it would be beneficial to distinguish the two concepts through naming throughout (although I'm uncertain that bypass and pending are the right names).

About the deprecation warning, it is unclear to me how that could be done in a forward-compatible manner (without also introducing a new name for the "new" pending concept). That is, the warning would have to be produced as soon as pending is used (as an existing test suite might rely on the pending tests not being executed), but there is no way of acknowledging that you actually want the 3.0 behaviour. Instead you have to resort to the old style it { pending { ... } } to avoid the warning and get the 3.0 behaviour.

Thus, my previous suggestion was to introduce pending! for the new pending behaviour, although I really don't like that particular name.

grddev commented Dec 15, 2013

I could probably take a stab at it, but from my point of view it seems a bit surprising to have a pending block that behaves differently to it "...", :pending and it "...", :pending => true. To me it would seem more natural to clearly split the two concepts

  • bypass: ignore the tests completely (xit and friends, and current pending)
  • pending: specify future desired behavior (current it with pending block)

I think it would be beneficial to distinguish the two concepts through naming throughout (although I'm uncertain that bypass and pending are the right names).

About the deprecation warning, it is unclear to me how that could be done in a forward-compatible manner (without also introducing a new name for the "new" pending concept). That is, the warning would have to be produced as soon as pending is used (as an existing test suite might rely on the pending tests not being executed), but there is no way of acknowledging that you actually want the 3.0 behaviour. Instead you have to resort to the old style it { pending { ... } } to avoid the warning and get the 3.0 behaviour.

Thus, my previous suggestion was to introduce pending! for the new pending behaviour, although I really don't like that particular name.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 16, 2013

Member

To me it would seem more natural to clearly split the two concepts

Hmm...this split doesn't describe how I use or think of uses of pending. That said, I can see some confusion around :pending metadata being different from the pending method. If we were going to create two different terms, I would probably advocate for skip (which does not run the example) and pending (which does, but expects the example to fail).

About the deprecation warning, it is unclear to me how that could be done in a forward-compatible manner (without also introducing a new name for the "new" pending concept). That is, the warning would have to be produced as soon as pending is used (as an existing test suite might rely on the pending tests not being executed), but there is no way of acknowledging that you actually want the 3.0 behaviour. Instead you have to resort to the old style it { pending { ... } } to avoid the warning and get the 3.0 behaviour.

Thus, my previous suggestion was to introduce pending! for the new pending behaviour, although I really don't like that particular name.

I'm reaallly not a fan of pending!. Using pending appeals to me because this:

describe MyClass do
  pending "does something" do
    do_something
  end
end

...looks so similar to:

describe MyClass do
  it "does something" do
    pending "waiting on an upstream fix" do
      do_something
    end
  end
end

I think the only way to introduce this "properly" would be a config option but I don't think I want to add that complexity. Instead, I think it's legit to simply change the semantics of ExampleGroup.pending in 3.0, and have it print a warning in 2.99 like so:

WARNING: the semantics of `RSpec::Core::ExampleGroup#pending` are changing in RSpec 3.
In RSpec 2.x, it caused the example to be skipped.  In RSpec 3, the example will still be run but
is expected to fail, and will be marked as a failure (rather than as pending) if the example passes,
just like how `pending` with a block from within an example already works.

To keep the same skip semantics, change `pending` to `xit`.  Otherwise, if you want the
new RSpec 3 behavior, you can safely ignore this warning and continue to upgrade to
RSpec 3 without addressing it.

It's not perfect, but I'd be OK with that option. I don't want us to wind up with a non-ideal API simply because of upgrade concerns.

Member

myronmarston commented Dec 16, 2013

To me it would seem more natural to clearly split the two concepts

Hmm...this split doesn't describe how I use or think of uses of pending. That said, I can see some confusion around :pending metadata being different from the pending method. If we were going to create two different terms, I would probably advocate for skip (which does not run the example) and pending (which does, but expects the example to fail).

About the deprecation warning, it is unclear to me how that could be done in a forward-compatible manner (without also introducing a new name for the "new" pending concept). That is, the warning would have to be produced as soon as pending is used (as an existing test suite might rely on the pending tests not being executed), but there is no way of acknowledging that you actually want the 3.0 behaviour. Instead you have to resort to the old style it { pending { ... } } to avoid the warning and get the 3.0 behaviour.

Thus, my previous suggestion was to introduce pending! for the new pending behaviour, although I really don't like that particular name.

I'm reaallly not a fan of pending!. Using pending appeals to me because this:

describe MyClass do
  pending "does something" do
    do_something
  end
end

...looks so similar to:

describe MyClass do
  it "does something" do
    pending "waiting on an upstream fix" do
      do_something
    end
  end
end

I think the only way to introduce this "properly" would be a config option but I don't think I want to add that complexity. Instead, I think it's legit to simply change the semantics of ExampleGroup.pending in 3.0, and have it print a warning in 2.99 like so:

WARNING: the semantics of `RSpec::Core::ExampleGroup#pending` are changing in RSpec 3.
In RSpec 2.x, it caused the example to be skipped.  In RSpec 3, the example will still be run but
is expected to fail, and will be marked as a failure (rather than as pending) if the example passes,
just like how `pending` with a block from within an example already works.

To keep the same skip semantics, change `pending` to `xit`.  Otherwise, if you want the
new RSpec 3 behavior, you can safely ignore this warning and continue to upgrade to
RSpec 3 without addressing it.

It's not perfect, but I'd be OK with that option. I don't want us to wind up with a non-ideal API simply because of upgrade concerns.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Dec 16, 2013

Member

why not give it a different name? In cucumber land this is "wip"...

Member

JonRowe commented Dec 16, 2013

why not give it a different name? In cucumber land this is "wip"...

@markijbema

This comment has been minimized.

Show comment
Hide comment
@markijbema

markijbema Dec 24, 2013

Hmm, I use pending also for "this test is pending a fix or removal". I would use this for instance for an integration test which suddenly fails randomly on the ci environment.

I really dislike the suggested warning. That way there is no easy way to ensure you're actually compatible. If I have to wade through dozens of false warnings to find what I do need to fix, I might as well just upgrade and see what fails.

Hmm, I use pending also for "this test is pending a fix or removal". I would use this for instance for an integration test which suddenly fails randomly on the ci environment.

I really dislike the suggested warning. That way there is no easy way to ensure you're actually compatible. If I have to wade through dozens of false warnings to find what I do need to fix, I might as well just upgrade and see what fails.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Jan 25, 2014

Member

@markijbema we only need to show the warning once on first usage - we already have code that does this.

Member

xaviershay commented Jan 25, 2014

@markijbema we only need to show the warning once on first usage - we already have code that does this.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Jan 25, 2014

Member

@markijbema and also you can just replace them all with xit to ensure you are compatible.

Member

xaviershay commented Jan 25, 2014

@markijbema and also you can just replace them all with xit to ensure you are compatible.

@ghost ghost assigned xaviershay Jan 25, 2014

xaviershay added a commit that referenced this issue Jan 26, 2014

Pending blocks will now be executed and marked as failed if they
succeed.

This is a backwards incompatible change that makes the behaviour of a
top-level `pending` call the sames as one used within an `it` block.

The old "never run this example" behaviour is still available via the
`xit` method or the `:skip` metadata option.

Implements #1208.

xaviershay added a commit that referenced this issue Jan 26, 2014

Pending blocks will now be executed and marked as failed if they
succeed.

This is a backwards incompatible change that makes the behaviour of a
top-level `pending` call the sames as one used within an `it` block.

The old "never run this example" behaviour is still available via the
`xit` method or the `:skip` metadata option.

Implements #1208.

xaviershay added a commit that referenced this issue Jan 26, 2014

Pending blocks will now be executed and marked as failed if they
succeed.

This is a backwards incompatible change that makes the behaviour of a
top-level `pending` call the sames as one used within an `it` block.

The old "never run this example" behaviour is still available via the
`xit` method or the `:skip` metadata option.

Implements #1208.

xaviershay added a commit that referenced this issue Jan 26, 2014

Pending blocks will now be executed and marked as failed if they
succeed.

This is a backwards incompatible change that makes the behaviour of a
top-level `pending` call the sames as one used within an `it` block.

The old "never run this example" behaviour is still available via the
`xit` method or the `:skip` metadata option.

Implements #1208.
@grddev

This comment has been minimized.

Show comment
Hide comment
@grddev

grddev Jan 27, 2014

For 2.99, I still think it would be important to have a forward-compatible (warning-free) solution. That is, there should be something I could write in 2.99 that would have the 3.0 pending behaviour, but without rendering any deprecation-warnings.

The obvious way to do this would be to introduce a new alias for the concept, but this is really suboptimal if pending is the preferred name.

Perhaps the best solution would be to provide a (temporary) configuration-flag for 2.99 to turn on the new behaviour (whereas 3.0 makes it the default)?

grddev commented Jan 27, 2014

For 2.99, I still think it would be important to have a forward-compatible (warning-free) solution. That is, there should be something I could write in 2.99 that would have the 3.0 pending behaviour, but without rendering any deprecation-warnings.

The obvious way to do this would be to introduce a new alias for the concept, but this is really suboptimal if pending is the preferred name.

Perhaps the best solution would be to provide a (temporary) configuration-flag for 2.99 to turn on the new behaviour (whereas 3.0 makes it the default)?

@markijbema

This comment has been minimized.

Show comment
Hide comment
@markijbema

markijbema Jan 27, 2014

If you warn on pending blocks (always) you would have forward-compatible behaviour. All 2.* pending blocks should be xit blocks, and switching from pending inline to pending blocks is optional. When upgraded to 3, you can convert inline pendings to block pendings.

If you warn on pending blocks (always) you would have forward-compatible behaviour. All 2.* pending blocks should be xit blocks, and switching from pending inline to pending blocks is optional. When upgraded to 3, you can convert inline pendings to block pendings.

@grddev

This comment has been minimized.

Show comment
Hide comment
@grddev

grddev Jan 27, 2014

Excellent. That in combination with the RunPending module above would provide for a reasonably smooth transition.

grddev commented Jan 27, 2014

Excellent. That in combination with the RunPending module above would provide for a reasonably smooth transition.

xaviershay added a commit that referenced this issue Feb 1, 2014

Pending blocks will now be executed and marked as failed if they
succeed.

This is a backwards incompatible change that makes the behaviour of a
top-level `pending` call the sames as one used within an `it` block.

The old "never run this example" behaviour is still available via the
`xit` method or the `:skip` metadata option.

Implements #1208.

xaviershay added a commit that referenced this issue Feb 2, 2014

Execute pending examples and mark as failed if they succeed.
This is a backwards incompatible change that makes the behaviour of
pending consisent with the current behaviour for when it is called
inside an example with a block.

The old "never run this example" behaviour is still available via the
"x" family of methods, the `:skip` metadata option, or the new `skip`
method.

Implements #1208.

@yujinakayama yujinakayama referenced this issue in yujinakayama/transpec Feb 8, 2014

Closed

Support conversion of pending examples #38

xaviershay added a commit that referenced this issue Feb 9, 2014

Execute pending examples and mark as failed if they succeed.
This is a backwards incompatible change that makes the behaviour of
pending consisent with the current behaviour for when it is called
inside an example with a block.

The old "never run this example" behaviour is still available via the
"x" family of methods, the `:skip` metadata option, or the new `skip`
method.

Implements #1208.

xaviershay added a commit that referenced this issue Feb 9, 2014

Execute pending examples and mark as failed if they succeed.
This is a backwards incompatible change that makes the behaviour of
pending consisent with the current behaviour for when it is called
inside an example with a block.

The old "never run this example" behaviour is still available via the
"x" family of methods, the `:skip` metadata option, or the new `skip`
method.

Implements #1208.

xaviershay added a commit that referenced this issue Feb 9, 2014

Make all uses of pending consistent.
* 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.
@markijbema

This comment has been minimized.

Show comment
Hide comment
@markijbema

markijbema Feb 21, 2014

I think this issue has been resolved by @xaviershay 's pull request, right? At least 2.99.beta2 gave me warnings about this exact feature.

I think this issue has been resolved by @xaviershay 's pull request, right? At least 2.99.beta2 gave me warnings about this exact feature.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Feb 21, 2014

Member

It has indeed. Thanks for the reminder to close this!

Member

myronmarston commented Feb 21, 2014

It has indeed. Thanks for the reminder to close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment