oesmith / puffing-billy

A rewriting web proxy for testing interactions between your browser and external sites. Works with ruby + rspec.
MIT License
656 stars 170 forks source link

Silent fail when error is raised inside a and_return proc #249

Closed 23tux closed 5 years ago

23tux commented 6 years ago

I have the following code that intercepts every request:

  proxy.stub(/(.*)/).and_return(proc { binding.pry; raise "error" })

Unfortunately, the error raise "error" is ignored when I visit a page. I get to the debugger, and when I continue from that point, it fails silently.

IMHO should the error be passed to RSpec where I can decide what to do with that error.

I also have the the eventmachine error handler enabled with EM.error_handler { |e| raise e }

EDIT:

I had a look into billy, and it seems like the Eventmachine error handler is overwritten here https://github.com/oesmith/puffing-billy/blob/master/lib/billy/proxy.rb#L71 Furthermore, even when I comment out Billy's error handler, every error gets rescued here https://github.com/oesmith/puffing-billy/blob/master/lib/billy/handlers/request_handler.rb#L31 So, throwing an error in the block has no effect overall.

Is this behaviour intended?

ronwsmith commented 6 years ago

Yes, this is intended behavior. I would say a vast majority of cases would want a silent failure if an error is raised by something fetched from the browser as the browser has to consume some response. If you're testing something in there, seems like it would be better in a jasmine test or similar.

twalpole commented 6 years ago

@23tux It can't work the way you want because puffing-billy gets involved in the communications between the browser and your app while the test is running, it has no connection with the actual RSpec driven tests other than configuration. Additionally when Ajax requests and redirections occur the tests don't necessarily have any knowledge of those occurring unless you're actively checking for page content changes or url changes.

One way to get some of what you want would be to log every failure somewhere and then have a final task that runs at the end of your tests that checks the log. Another would be to add an endpoint to your app under test (that is only available in the test environment) that raises an error inside the app. You can then call that endpoint (with whatever info you want to pass back) from your puffing billy stub whenever you want failure info returned. The error raised inside the app will then be detected by Capybara (assuming Capybara.raise_server_errors == true) and raised in the test at its next attempted action.

23tux commented 6 years ago

Thanks for your answers! I get your point why the error isn't passed to capybara. But then, I find it confusing when this line appears in the documentation

EM.error_handler { |e| raise e }

This looks like you could get every error from EventMachine into your test. But as you said, this isn't possible. To get this to work, you would have to change this line from fail to raise https://github.com/oesmith/puffing-billy/blob/master/lib/billy/proxy_connection.rb#L64 (not sure why, maybe EM treats a fail different from a raise; best practises like Rubocop always suggest to avoid fail) and you would have to put the EM.error_handler { |e| raise e } AFTER you stub something.

If it is not possible to use those errors, I would a) try to find a way to use them (would appreciate some help and be happy to make a PR) or b) remove this parts from the README, so that it doesn't confuse other users.

ronwsmith commented 5 years ago

@23tux Were you able to get your suggested workaround working? If so, please submit a PR.

23tux commented 5 years ago

@ronwsmith unfortunately no

ronwsmith commented 5 years ago

@23tux would you like this keep this issue open?

23tux commented 5 years ago

@ronwsmith nope it can be closed. We ended up removing billy, didn't work out for us