Modified example not to recognize first non string object of argument as description

[eisukeyeongjo]

If this patch is unnecessary and non string object is acceptable for description, I'll close this PR. But I think it is not and I created this PR because with this patch, RSpec seems better behavior.

a case of rspec below

context do
  it do
    # without description nor option
    expect(true).to eq true
  end

  it 'only description' do
    expect(true).to eq true
  end

  it :pending do
    # Only pending option without reason
    expect(true).to eq false
  end

  it pending: 'only pending option' do
    expect(true).to eq false   
  end  

  it 'description with option', pending: 'some reason' do
    expect(true).to eq false
  end
end

before

Two examples failed because pending option was recognized as description and invalid.

$ rspec test.rb --format documentation

  is expected to eq true
  only description
  pending (FAILED - 1)
  {:pending=>"only pending option"} (FAILED - 2)
  description with option (PENDING: some reason)

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) description with option
     # some reason
     Failure/Error: expect(true).to eq false

       expected: false
            got: true

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -false
       +true

     # ./test.rb:21:in `block (2 levels) in <top (required)>'

Failures:

  1) pending
     Failure/Error: expect(true).to eq false

       expected: false
            got: true

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -false
       +true

     # ./test.rb:13:in `block (2 levels) in <top (required)>'

  2) {:pending=>"only pending option"}
     Failure/Error: expect(true).to eq false

       expected: false
            got: true

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -false
       +true

     # ./test.rb:17:in `block (2 levels) in <top (required)>'

Finished in 0.01111 seconds (files took 0.06421 seconds to load)
5 examples, 2 failures, 1 pending

Failed examples:

rspec ./test.rb:11 # pending
rspec ./test.rb:16 # {:pending=>"only pending option"}

after

Every pending option is valid because example recognized description only if the first argument is string object.

$ rspec test.rb --format documentation

  is expected to eq true
  only description
  is expected to eq false (PENDING: No reason given)
  is expected to eq false (PENDING: only pending option)
  description with option (PENDING: some reason)

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) is expected to eq false
     # No reason given
     Failure/Error: expect(true).to eq false

       expected: false
            got: true

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -false
       +true

     # ./test.rb:13:in `block (2 levels) in <top (required)>'

  2) is expected to eq false
     # only pending option
     Failure/Error: expect(true).to eq false

       expected: false
            got: true

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -false
       +true

     # ./test.rb:17:in `block (2 levels) in <top (required)>'

  3) description with option
     # some reason
     Failure/Error: expect(true).to eq false

       expected: false
            got: true

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -false
       +true

     # ./test.rb:21:in `block (2 levels) in <top (required)>'

Finished in 0.01149 seconds (files took 0.07832 seconds to load)
5 examples, 0 failures, 3 pending

[pirj]

Honestly, I like this change, as it enforces the specification - metadata comes after the doc_string and the first argument can’t be metadata.

However, now we accept eg a symbol as the first argument, and it’s treated as metadata.

The current behaviour can certainly cause unwanted effects, but how bad it can be? Adding a pending example with example code that really passes while it should have failed?

I can think of examples that use symbols as a docstring. Now, those symbols will be treated as metadata, with unpredictable effects on the spec.

Or examples that are passed class literals, backticks-quoted “strings”, variables that can contain anything. Those, previously converted to a string and used as a doctring, now will be attempted to be parsed as metadata, and will fail.

Those three are breaking changes, and there can be more.

We can accept a breaking change like this in an upcoming 4.0, but it would be a broader change, with warnings added to 3.99, and a stricter version of thus PR to disallow non-string non-metadata as the first argument plus specs/docs changes in 4.0. Would you be interested in contributing this?

@JonRowe WDYT?

[eisukeyeongjo]

@pirj Thank you for your comment! And I'm sorry, there wasn't enough consideration given to the impact this fix would have.

Those three are breaking changes, and there can be more.

I'm totally agree with you. If this new specification is acceptable, I'm interested in contributing it and its related tasks in upcoming new major version.

[JonRowe]

The publically documented API is that the first argument is a docstring, if you want to provide metadata you can do so after that either in the form of the symbol key shortcut or in the form of a hash but only after a docstring. We have slowly been trying to push people into writing docstrings for their tests, the explictness is a good thing and one of the reasons for RSpec is to write descriptive tests.

So I'm not in favour of being able to omit a docstring to pass just metadata, the current behaviour is correct in that its considering the first argument as the doc string. Obviously for a symbol that works just fine, and the hash is a little weird but might well have valid use cases.

I think I'm leaning towards closing this, and making this a hard requirement of a string in 4.x...

[pirj]

@eisukeyeongjo, awesome. 4.0-dev branch is for 4.0, and this is an example of warnings for 3.99.

To estimate the impact, it might be possible to use some static code querying against https://github.com/pirj/real-world-rspec. I’ll try that.

[eisukeyeongjo]

@pirj Thank you so much!! So I'll close this PR and create new issue for the new specification, then create new one!