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

Add #fits and #xits methods #79

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bazay
Copy link

@bazay bazay commented Nov 20, 2020

RSpec core library has a convention of prepending 'f' and 'x' to it's methods to support running certain blocks or tests with :focus or :skip tags. This commit aims to bring rspec-its inline with this convention by making #fits and #xits methods available to the spec writer.

spec/rspec/its_spec.rb Outdated Show resolved Hide resolved
@bazay
Copy link
Author

bazay commented Nov 23, 2020

I'm also happy to update README with both methods with the commit resolving this discussion:
#79 (comment)

lib/rspec/its/version.rb Outdated Show resolved Hide resolved
@JonRowe
Copy link
Member

JonRowe commented Nov 24, 2020

Thanks for the suggestion, I'm not totally convinced I want to expand this library, it exists for legacy reasons (it was extracted from rspec) and we don't encourage it's use.

@bazay bazay force-pushed the add-fits-and-xits-aliases branch 2 times, most recently from 8cc4ae1 to 68fd1b0 Compare November 24, 2020 17:27
@bazay
Copy link
Author

bazay commented Nov 24, 2020

I know a lot of teams who still use this gem in everyday use so it still has value beyond usage in legacy apps.
Ultimately I think it should be up to the individual developer to decide or at least be given the choice :)

Perhaps a solution would be to allow new contributions to be added on a new major release version (e.g. v2.0.0) to clearly extend on the original "supporting for legacy reasons" purpose of this gem.

@JonRowe
Copy link
Member

JonRowe commented Nov 25, 2020

I know a lot of teams who still use this gem in everyday use so it still has value beyond usage in legacy apps.
Ultimately I think it should be up to the individual developer to decide or at least be given the choice :)

Indeed it is, which is why the gem even exists, but thats different from wether the RSpec team want to accept new features which creates additional maintenance burden for us.


it { expect(subject).to include('2 examples, 0 failures, 1 pending') }
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Please take a look at how rspec-core tests example groups, this isn't a solution we would want to merge.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @JonRowe I've updated the specs. Let me know if they're okay

Copy link
Member

Choose a reason for hiding this comment

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

I guess Jon meant something more like this.

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't copy the simplicity of that example exactly, because we're working with it (example) vs fits (describe) so they need to be handled differently, reflected in the updated spec. Not sure if this is as clear as what I had before but it works I guess 🙁

@bazay bazay force-pushed the add-fits-and-xits-aliases branch 5 times, most recently from 36aa30d to 97d9e04 Compare December 2, 2020 15:44
end

expect(ex_its.examples.first.metadata[:skip]).to be_falsey
expect(ex_xits.examples.first.metadata[:skip]).to be_truthy
Copy link
Author

@bazay bazay Dec 4, 2020

Choose a reason for hiding this comment

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

FYI #its returns a RSpec describe block rather than the actual example, which is why we need to call examples on that describe block (always containing just 1) then selecting our its example.
https://github.com/rspec/rspec-its/pull/79/files#diff-39939cafdb9cee9c4840e8adc3819e0a45dafa210f75a51a34ed5bdb74c7ef4fL123

Copy link
Member

Choose a reason for hiding this comment

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

Isn't metadata applied to this example group itself? Can you check example group metadata instead?

Copy link
Author

@bazay bazay Dec 6, 2020

Choose a reason for hiding this comment

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

That doesn't appear to be the case:

pry(#<RSpec::ExampleGroups::RSpecIts::Fits>)> ex_fits.metadata
=> {:execution_result=>#<RSpec::Core::Example::ExecutionResult:0x007fbdf61f6268>,
 :block=>#<Proc:0x007fbdf61f64c0@/Users/baron/Developer/personal/rspec-its/lib/rspec/its.rb:123>,
 :description_args=>["class"],
 :description=>"class",
 :full_description=>" class",
 :described_class=>nil,
 :file_path=>"./spec/rspec/its_spec.rb",
 :line_number=>400,
 :location=>"./spec/rspec/its_spec.rb:400",
 :parent_example_group=>
  {:execution_result=>#<RSpec::Core::Example::ExecutionResult:0x007fbdf61fe648>,
   :block=>#<Proc:0x007fbdf61fe8c8@/Users/baron/Developer/personal/rspec-its/spec/rspec/its_spec.rb:396>,
   :description_args=>[nil],
   :description=>"",
   :full_description=>"",
   :described_class=>nil,
   :file_path=>"./spec/rspec/its_spec.rb",
   :line_number=>396,
   :location=>"./spec/rspec/its_spec.rb:396"}}

and on the example group:

pry(#<RSpec::ExampleGroups::RSpecIts::Fits>)> ex_fits.example_group.metadata
=> {:execution_result=>#<RSpec::Core::Example::ExecutionResult:0x007fbdf626e560>,
 :block=>nil,
 :description_args=>[nil],
 :description=>"",
 :full_description=>" class ",
 :described_class=>nil,
 :file_path=>"(pry)",
 :line_number=>8,
 :location=>"(pry):8",
 :parent_example_group=>
  {:execution_result=>#<RSpec::Core::Example::ExecutionResult:0x007fbdf61f6268>,
   :block=>#<Proc:0x007fbdf61f64c0@/Users/baron/Developer/personal/rspec-its/lib/rspec/its.rb:123>,
   :description_args=>["class"],
   :description=>"class",
   :full_description=>" class",
   :described_class=>nil,
   :file_path=>"./spec/rspec/its_spec.rb",
   :line_number=>400,
   :location=>"./spec/rspec/its_spec.rb:400",
   :parent_example_group=>
    {:execution_result=>#<RSpec::Core::Example::ExecutionResult:0x007fbdf61fe648>,
     :block=>#<Proc:0x007fbdf61fe8c8@/Users/baron/Developer/personal/rspec-its/spec/rspec/its_spec.rb:396>,
     :description_args=>[nil],
     :description=>"",
     :full_description=>"",
     :described_class=>nil,
     :file_path=>"./spec/rspec/its_spec.rb",
     :line_number=>396,
     :location=>"./spec/rspec/its_spec.rb:396"}}}

Copy link
Member

Choose a reason for hiding this comment

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

You are right, metadata is only set on the example itself

example(nil, *options, &block)

lib/rspec/its.rb Outdated Show resolved Hide resolved
lib/rspec/its.rb Outdated Show resolved Hide resolved
end

expect(ex_its.examples.first.metadata[:skip]).to be_falsey
expect(ex_xits.examples.first.metadata[:skip]).to be_truthy
Copy link
Member

Choose a reason for hiding this comment

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

Isn't metadata applied to this example group itself? Can you check example group metadata instead?

spec/spec_helper.rb Outdated Show resolved Hide resolved
spec/rspec/its_spec.rb Outdated Show resolved Hide resolved

it { expect(subject).to include('2 examples, 0 failures, 1 pending') }
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I guess Jon meant something more like this.

@bazay
Copy link
Author

bazay commented Dec 7, 2020

Looks like something unusual happened whilst running the test-suite. Perhaps Travis needs to be kicked 👢

@pirj
Copy link
Member

pirj commented Dec 12, 2020

Perhaps Travis

We're moving other repos to GHA. Not focused on rspec-its/rspec-collection-matchers, you can lend a hand if you like.

@bazay
Copy link
Author

bazay commented Dec 14, 2020

Sure I can take a look

@bazay
Copy link
Author

bazay commented Dec 14, 2020

I've added GHA here: #80

RSpec core library has a convention of prepending 'f' and 'x' to it's methods to support running certain blocks or tests with :focus or :skip tags. This commit aims to bring rspec-its inline with this convention by making #fits and #xits methods available to the spec writer.
@bazay
Copy link
Author

bazay commented Jan 6, 2021

Hey @JonRowe thanks for merging in your GHA changes. Looks like Travis is having some trouble installing bundler, though not sure why as gem install bundler -v '< 2' isn't specified in the .travis.yml from what I can tell 🤔

@bazay
Copy link
Author

bazay commented Jan 12, 2021

Hey @JonRowe @pirj any update on this - is there anything you want me to do on my end? :)

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Thanks for handling this!

@bazay
Copy link
Author

bazay commented Jan 26, 2021

ping @JonRowe 😬

@JonRowe
Copy link
Member

JonRowe commented Jan 26, 2021

Thanks for the suggestion, I'm not totally convinced I want to expand this library, it exists for legacy reasons (it was extracted from rspec) and we don't encourage it's use.

ping @JonRowe 😬

My original reticence remains, I don't have time to maintain this gem and release new features for it, so this is far down on my to do list.

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.

3 participants