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

doc_string which is the first argument of example, must be string object #3072

Open
2 tasks
eisukeyeongjo opened this issue Mar 16, 2024 · 3 comments
Open
2 tasks
Milestone

Comments

@eisukeyeongjo
Copy link

eisukeyeongjo commented Mar 16, 2024

doc_string which is the first argument of example, must be string object

RSpec example recognize the first argument as the description.
But when even symbol or hash object which set intended as metadata is given in the first argument, it is recognized as the description and metadata does not work.
I want to add some change to keep doc_string a string object.

Your environment

  • Ruby version: 3.3.0
  • rspec-core version: 3.1.3

Steps to reproduce

prepare rspec test below

$ vi test.rb
$ cat test.rb
context do
  it :pending do
    expect(true).to eq false
  end

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

Actual behavior

metadata is recognized as description and pending does not work, failing two examples

$ bundle exec rspec  test.rb --format documentation

  pending (FAILED - 1)
  {:pending=>"some reason"} (FAILED - 2)

Failures:

  1) pending
     Failure/Error: expect(true).to eq false
     
       expected: false
            got: true
     
       (compared using ==)
     
       Diff:
       @@ -1 +1 @@
       -false
       +true
       
     # ./test.rb:14:in `block (2 levels) in <top (required)>'

  2) {:pending=>"some reason"}
     Failure/Error: expect(true).to eq false
     
       expected: false
            got: true
     
       (compared using ==)
     
       Diff:
       @@ -1 +1 @@
       -false
       +true
       
     # ./test.rb:18:in `block (2 levels) in <top (required)>'

Finished in 0.01336 seconds (files took 0.08613 seconds to load)
2 examples, 2 failures

Failed examples:

rspec ./test.rb:13 # pending
rspec ./test.rb:17 # {:pending=>"some reason"}

Expected behavior

To prevent metadata as first argument which does not work properly, it raises ArgumentError like below when execute each example above whose first argument is not nil or string object.

ArgumentError:
  `:pending` is not acceptable for doc_string.
  it must be a string.

Note

First, I created PR #3074 not to set doc_string when first argument is metadata.
But I think that keeping the first argument of example doc_string is much more appropriate.
(In the first place, as commented here it's the specification.)

Related Pull Request

I proposed this new specification in the PR below,
After the discussion, we decided to add this in upcoming 4.x and add warning in 3.99

#3071

What to do

@pirj
Copy link
Member

pirj commented Mar 27, 2024

@JonRowe do you agree with the expected behaviour section here?

doc_string which is the first argument of example, must be string object

It confuses me when the first argument is not a string and we accept it as metadata.
It seems to be a move in the wrong direction, bending to the pressure of existing specs that pass non-strings and expect that to work.

To me, the “must” in the “must be a string object” means we raise an argument error when it’s not.

In my defense, it :pending can become just pending, even though it’s just a special case, and it :clean_db doesn’t have/deserve such an alias.

I can foresee misuse and workarounds like it “”, :clean_db do.

I find it hard to justify the it pending: “not implemented yet” do as it gives no information on what wasn’t implemented exactly. But again, this is just a special case. Another example would be it platform: :ios, but that probably should be a context.

I’d love to see more examples of what we are going to break if we go with a more strict approach.

It’s more of a gut feeling, but going with accepting the first argument as metadata where it previously wasn’t, makes me uncomfortable when thinking of side effects that might be introduced by such metadata.

@eisukeyeongjo
Copy link
Author

@pirj
Sorry for confusing you with this issue.
And I apologize for my lack of thought.
I understand what you mentioned above.
Certainly keeping the first argument doc_sting seems a right direction.
If you do not mind, i will re-write this issue and create new pull request.

What happens to this issue may depend on Mr. @JonRowe 's reply.
But I will try to face this issue a little more.

@pirj
Copy link
Member

pirj commented Apr 17, 2024

Sounds good, and no worries. Thanks for your contributions!

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

No branches or pull requests

2 participants