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.

Sign up for GitHub

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

Jump to bottom

Clarify typhoeus callbacks tests #935

Merged
merged 26 commits into from May 5, 2022
Merged

Clarify typhoeus callbacks tests #935

merged 26 commits into from May 5, 2022

Conversation

nellirx
Copy link
Contributor

@nellirx nellirx commented Apr 24, 2022

  • checking what is yielded in a clearer way
  • documented suspicious behaviour with on_body callback when running single Request instance multiple times, i.e. request.run (commented with FIXME)

@olleolleolle
Copy link
Member

Looks like a warning that can be fixed - to pass the "no Warnings allowed" part of the suite.

olleolleolle
olleolleolle reviewed Apr 25, 2022
View reviewed changes
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Cool that you got it passing! Thanks for all the work!

I can see FIXME notes in the specs, are they "to be fixed in this commit" or are they for the future? If they are for the future, I'd like it very much if they had a run-on comment like "FIXME so that X doesn't Y when Z happens", to clarify what the FIXME refers to.

Thanks again for taking this on!

spec/lib/vcr/library_hooks/typhoeus_spec.rb Outdated
context 'after recording cassette (first request)' do
before { on_body }

it { expect(on_body).to have_attributes(body: 'Localhost responseLocalhost response') } # FIXME
Copy link
Member

Choose a reason for hiding this comment

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

Curious about these: is this change exposing the re-recording issue, or causing it? Should the FIXME be repaired in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention was to expose what to me looks like an issue.
I'd prefer to address it separately please, if that's alright of course.

@nellirx
Copy link
Contributor Author

nellirx commented Apr 27, 2022

If they are for the future, I'd like it very much if they had a run-on comment like "FIXME so that X doesn't Y when Z happens", to clarify what the FIXME refers to

@olleolleolle thank you for the feedback! I have clarified what the FIXME refers to.

@nellirx
Copy link
Contributor Author

nellirx commented May 1, 2022

@olleolleolle I decided to remove "FIXME" related tests to avoid confusion

  1. In original tests we didn't use single instance of request, so, if we keep them that way, I believe no fixing would be necessary just yet;
  2. the tests with FIXME pass, which may suggest that it is intentional behaviour, while FIXME suggest the opposite, it might be confusing;
  3. I'll have a close look into issue and create separate PR with those tests if I find a way to fix/improve it.

Hope that's alright. Please let me know if any questions/suggestions.

Thank you!

@olleolleolle olleolleolle mentioned this pull request May 2, 2022
Merged
@olleolleolle
Copy link
Member

olleolleolle commented May 2, 2022 •
edited

@nellirx Could you rebase this on top of latest? #937 made the red checks green.

nellirx added 24 commits May 2, 2022 13:14
@nellirx
Clarify test description to better reflect what it does
1583421
@nellirx
Add missing assertions
59cef24
@nellirx
Split tests per callback
16257c5 
Leaving assertions for body in on_headers callback test, because I don't
yet know if body affected by on_headers callback or not.
@nellirx
Confirm on_headers doesn't affect response body
3426b16 
Only streaming responses has no body, because those sent in chunks to
the on_body callback.
@nellirx
Make expectations more explicit for on_headers callback
ee159dd
@nellirx
Remove redundant assertion
58407ba
@nellirx
Remove redundant variables
f82abb9
@nellirx
Simplify with single assertion per test
001b17c
@nellirx
Improve style consistency
36476ed
@nellirx
Remove unused code
7978208
@nellirx
Simplify usage of use_cassette block
1676529
@nellirx
Narrow the scope of use_cassette block
fd8a17d 
Easier to see action which makes request and therefore requires cassette
@nellirx
Document expected arguments
4f0cdde
@nellirx
Clarify intention
b1115c5 
Expecting replayed headers to match with originally recorded headers
@nellirx
Make easier to spot related tests
084fef2
@nellirx
Extract duplicate code
488d666
@nellirx
Simplify
6f6fa4e
@nellirx
Use consistent matchers to verify response
54b8af9 
Regardless of where response occurs, i.e. insdie or outside of callback.
@nellirx
Clarify expected arguments for on_body callback
386f24a
@nellirx
Remove redundant assertions
09ce2e0
@nellirx
Document observations for on_body callback on single request instance
fc26b22
@nellirx
Add more tests covering on_body returning :abort
f4db89f
@nellirx
Remove redundant test
717b5d1
@nellirx
Remove ambiguity as to what "&" means
08cc3cb 
Removes warning: `&' interpreted as argument prefix
@nellirx
Clarify what might need be fixed
789d2e3
@nellirx
Remove tests we aren't going to fix yet
e30d729 
Further investigation required to decide if we'd like to amend behavior
or to capture it as it is.
@nellirx nellirx force-pushed the clarify-typhoeus-callbacks-tests branch from 68c386a to e30d729 Compare May 2, 2022 12:15
olleolleolle
olleolleolle approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Thanks!

These are complicated tests, they are, but now they are cut up in their constituting parts.

Thanks for doing that!

@olleolleolle olleolleolle merged commit be6a782 into vcr:master May 5, 2022
@olleolleolle
Copy link
Member

@nellirx You did a lot of heavy lifting there - that's very helpful 🏆 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olleolleolle olleolleolle olleolleolle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nellirx @olleolleolle