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

Fix nested described class #1361

Merged
merged 3 commits into from Mar 4, 2014
Merged

Fix nested described class #1361

merged 3 commits into from Mar 4, 2014

Conversation

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Mar 2, 2014

Make described_class to return innermost described class.

It used to return the outermost described class. It doesn't
happen very often that there is more than one class being
described in the set of nested example groups (and we
generally recommend against it), but this was surprising behavior
to me.

Fixes #1114.

@myronmarston
Copy link
Member Author

@myronmarston myronmarston commented Mar 2, 2014

This needs a 2.99 deprecation for the case where it is changing. I'll work on that in a bit.

Loading

It used to return the outermost described class. It doesn't
happen very often that there is more than one class being
described in the set of nested example groups (and we
generally recommend against it), but this was surprising
behavior to me.

Fixes #1114.
@@ -150,7 +150,7 @@ def described_class
end
end

container_stack.reverse.each do |g|
container_stack.each do |g|
Copy link
Member

@JonRowe JonRowe Mar 2, 2014

Choose a reason for hiding this comment

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

Why? (Just curious)

Loading

Copy link
Member

@JonRowe JonRowe Mar 2, 2014

Choose a reason for hiding this comment

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

Wait... Is this all that's needed to make the describes reference the innermost class?

Loading

Copy link
Member Author

@myronmarston myronmarston Mar 2, 2014

Choose a reason for hiding this comment

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

Yep. This searches for a described class from the inner most group to the outermost group, rather than the reverse like before.

That said, I think I just realized a possible bug with this, related to the container_stack.each loop above this -- let me play a bit and see if the bug I'm thinking of exists.

Loading

In the last commit, I made inner `describe SomeClass`
example groups get `SomeClass` as the described_class.
The logic confusingly allowed a `:described_class` metadata
key in an outer group take precedence, which meant that the
behavior depended on whether or not `described_class` was
referenced from the outer group.

This fixes that, making it work consistently:

- Use `:described_class` is present.
- Otherwise, use the group's `describe` arg
  (unless it's something like a string).
- If neither of those, look in the parent and
  recurse.
@myronmarston
Copy link
Member Author

@myronmarston myronmarston commented Mar 2, 2014

I was right about there being a bug. Fix pushed. Want to re-review, @JonRowe?

Loading

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Mar 2, 2014

LGTM

Loading

@myronmarston
Copy link
Member Author

@myronmarston myronmarston commented Mar 2, 2014

Thanks for reviewing. Going to hold off merging until I have a 2.99 deprecation PR ready.

Loading

myronmarston added a commit that referenced this issue Mar 4, 2014
@myronmarston myronmarston merged commit f47711c into master Mar 4, 2014
1 check passed
Loading
@myronmarston myronmarston deleted the fix-nested-described-class branch Mar 4, 2014
pdswan added a commit to pdswan/rspec-rails that referenced this issue Mar 17, 2015
pdswan added a commit to pdswan/rspec-rails that referenced this issue Mar 17, 2015
pdswan added a commit to pdswan/rspec-rails that referenced this issue Mar 17, 2015
pdswan added a commit to pdswan/rspec-rails that referenced this issue Mar 18, 2015
penelopezone pushed a commit to rspec/rspec-rails that referenced this issue Dec 6, 2015
sebjacobs added a commit to futurelearn/rspec-rails that referenced this issue Mar 15, 2019
pirj added a commit to pirj/rspec-core that referenced this issue May 25, 2019
Fixes rspec#2627

Documentation for `described_class` was out of date, a change was made
in early 3.0 in rspec#1361, but was not reflected to the docs.

Also, documentation for implicit `subject` was off, even though there is
a feature describing how it behaves.
myronmarston added a commit that referenced this issue Aug 8, 2019
Fixes #2627

Documentation for `described_class` was out of date, a change was made
in early 3.0 in #1361, but was not reflected to the docs.

Also, documentation for implicit `subject` was off, even though there is
a feature describing how it behaves.
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this issue Oct 30, 2020
Fixes rspec#2627

Documentation for `described_class` was out of date, a change was made
in early 3.0 in rspec#1361, but was not reflected to the docs.

Also, documentation for implicit `subject` was off, even though there is
a feature describing how it behaves.
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this issue Oct 6, 2021
Fixes rspec/rspec-core#2627

Documentation for `described_class` was out of date, a change was made
in early 3.0 in rspec/rspec-core#1361, but was not reflected to the docs.

Also, documentation for implicit `subject` was off, even though there is
a feature describing how it behaves.

---
This commit was imported from rspec/rspec-core@e90f9b5.
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this issue Oct 6, 2021
Fixes rspec/rspec-core#2627

Documentation for `described_class` was out of date, a change was made
in early 3.0 in rspec/rspec-core#1361, but was not reflected to the docs.

Also, documentation for implicit `subject` was off, even though there is
a feature describing how it behaves.

---
This commit was imported from rspec/rspec-core@861986d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants