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

Bring in Shopify modifications #2

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

robforman
Copy link
Contributor

No description provided.

@robforman
Copy link
Contributor Author

Thanks for all the work on this, @peterjm.

Quick question- have you verified that it plays back without opening real connections? I ran tcpdump while running the specs and I see traffic when its supposed to be playing back from cassette. Is there a better way to test your changes?

rspec:
image

tcpdump (not supposed to see traffic):
image

@peterjm
Copy link

peterjm commented Mar 26, 2014

Interesting -- I hadn't used tcpdump; I'll take a look to see where the connection is coming from. Hopefully it's just an extra method that needs to be stubbed on the socket classes.

@peterjm
Copy link

peterjm commented Mar 26, 2014

Hmmmm... I wasn't able to reproduce the problem:

screen shot 2014-03-26 at 9 46 26 am

I'm a newbie to tcpdump, though.

And I agree that the way I'm checking for real http connections in the test isn't great. Maybe there's a system command we could try to use in the tests to detect real traffic?

@peterjm
Copy link

peterjm commented Mar 26, 2014

Also, I'd love to get this merged upstream, but there's a few caveats that I was going to address before sending a PR:

  1. The use of prepend in tcr.rb means these changes are Ruby 2 only. We could avoid it by monkey-patching and using an alias method chain on initialize
  2. The cassettes generated by these changes aren't backwards compatible
  3. I didn't do anything with the to_io method, which you were previously stubbing out. I might have broken functionality for what you were previously doing
  4. I was lazy about writing new specs for new functionality 😦

@robforman
Copy link
Contributor Author

Yeah, tcpdump is playing hard to get. You need to add -l to flush the output buffer so grep can see it. You also need -A to show the ascii representation. Here are the flags I used:

image

I agree we should look for a deeper hook to watch for live traffic. I'll do some digging.

As far as your caveats:

  1. I'm fine with Ruby 2.1- prepend is awesome. I'm already moving most of my projects over to Ruby 2.
  2. Cassette migration is sort of a pain. Maybe we can add some backwards compatibility flag for reading old cassettes. It shouldn't be hard since the old ones are simpler. But I also try to design tests in a way where the cassettes can be deleted and re-run. Not a showstopper for me.
  3. I saw that about to_io. Hard to say whether its broken yet. I found the issue with live traffic as I was testing this.
  4. Now is your chance!

@peterjm
Copy link

peterjm commented Mar 26, 2014

  1. Cool.
  2. Detecting old cassettes would be pretty easy -- old cassettes serialize to an array, and new ones are a hash, so we could check whether the serialized object is an array and wrap it accordingly. We'd also need to not fail on close and connect when replaying old cassettes.
  3. Thanks for the help with tcpdump -- I'll keep looking into the connection issue when I get a chance.
  4. Hopefully on Friday or the weekend :)

@gmalette
Copy link

gmalette commented Nov 9, 2016

Any chance this will get picked up again?

@robforman
Copy link
Contributor Author

The main issue was that the changes broke the network isolation somewhere along the way and it was sending traffic instead of stubbing it.

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.

5 participants