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

Use the specs scope as the cassette name when the description is empty #514

Merged
merged 1 commit into from Nov 16, 2015
Merged

Use the specs scope as the cassette name when the description is empty #514

merged 1 commit into from Nov 16, 2015

Conversation

tessi
Copy link
Contributor

@tessi tessi commented Oct 30, 2015

The scope remains relatively stable when new spec are inserted into the suite.
It's not as good as a description, but better than nothing.

Relates to (and possibly fixes) #378.
This patch helps a lot with it { is_expected.to be 'something' }-style specs.
Multiple of those specs overwrite each others cassettes, which is fixed by this patch.

#378 has a rather long discussion about which cassette name should be used in this case. I'd vote for the scope because

  • we don't have a description
  • the line number of the specs changes a lot when inserting new specs to the testsuite
  • there is no easy way to get the source code of the spec (one could use the method_source gem, but it has other limitations for example with dynamically defined specs)

The nested scope does not change when changing existing specs, or appending specs to the suite.

The scope remains relatively stable when new spec are inserted into the
suite.
It's not as good as a description, but better than nothing.
@tessi
Copy link
Contributor Author

tessi commented Nov 4, 2015

I could use the Proc's source (instead of the scope) by using the method_source gem if you prefer that.
Probably the method source should be hashed, so that we don't have to care about multi-line or too long sources.

metadata[:scoped_id]
else
metadata[:description]
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure the state of things is that metadata[:description] will either be a full string "something" or an empty string ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the case while I had my debugger in that line. I can play save and add a nil-check, though :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's best, unless we can be better up the line and add a place that does metadata[:description] = ... || ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to this method in the rspec core, the description can never be nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krainboltgreene is that info what you needed? Or shall I add a nil check?

@krainboltgreene
Copy link
Contributor

Sorry this took so long to respond, I wasn't getting notifications and now that's fixed. Great catch, I'll merge once I'm sure about that question.

@tessi
Copy link
Contributor Author

tessi commented Nov 6, 2015

@krainboltgreene thanks for answering :) Do you like the scope or the method_gem approach more? I can do either way.

@krainboltgreene
Copy link
Contributor

@tessi Scope is good enough. I'll keep a note for trying method_gem if people have problems.

krainboltgreene pushed a commit that referenced this pull request Nov 16, 2015
Use the specs scope as the cassette name when the description is empty
@krainboltgreene krainboltgreene merged commit 033a824 into vcr:master Nov 16, 2015
@krainboltgreene
Copy link
Contributor

@tessi Thanks!

@tessi tessi deleted the cassette_names_for_empty_descriptions branch November 17, 2015 15:15
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.

None yet

2 participants