Bubble up errors that aren't handled by applications/frameworks #660

Closed
wants to merge 2 commits into
from

Projects

None yet

7 participants

@raggi
Contributor
raggi commented Mar 6, 2012

Currently, people do things like make servers puts or use Thread.abort_on_exception = true to do this. That leads to other problems, so I added some code to cache the errors and re-raise in the testing thread.

In general this will display errors that are released / uncaught by frameworks (e.g. rails in test mode). Sometimes, where the first request doesn't error, but subsequent asset / ajax type requests do, it will raise those errors instead. Whilst this may be slightly odd, it will help with having a faster feedback loop most of the time.

Ideally it wouldn't rely on driver.rack_server. I'd actually prefer that session handled server booting when drivers request servers, however, this would be a larger refactor that is not appropriate for rolling up into these changes.

@raggi
Contributor
raggi commented Mar 6, 2012

See #661

@jnicklas
Collaborator
jnicklas commented Mar 6, 2012

We need to find a better way than only checking the errors when visit is called. Maybe it isn't, and in that case this whole feature is of no use. Unless I misunderstand how this works.

Maybe we can just collect all errors that are raised, and then have a hook method which would allow us to re-raise those exceptions. Something like Capybara.current_session.reraise_captured_expressions! or something (the name is dumb). And we would call this method from hooks in capybara/rspec and capybara/cucumber. What do you think about that?

@raggi
Contributor
raggi commented Mar 7, 2012

You can only raise a single exception, so trying to re-raise all simply can't be meaningful.

We could bundle them up in a single exception, and raise that, but, then the backtrace is less useful for people, and to be honest, it becomes a failed tool.

I didn't look in any detail, and I likely should, what other public API methods actually cause requests to be made? (n.b. I don't really count evaluate_script(window.location =) as something valid, as it's not portable across drivers - i.e. that'll attempt to run against about:blank in selenium).

I don't like the idea of adding hooks for specific test frameworks, that again damages portability. An exception is an exceptional condition, and should always raise. If we do something else, like "assert capybara.last_error.nil?" then we're just reimplementing the should not raise pattern, which is bad. It would also as mentioned above, lead to much worse UX.

The first request the browser makes is almost always the most critical. If that fails, other requests generally won't even be made. If a middling request (ajax) fails, we still need to know too.

@jnicklas
Collaborator
jnicklas commented Mar 7, 2012

click_link and click_button. Ajax requests can be caused by pretty much anything that Capybara does. Also, websockets or polling could cause an error even when the user does absolutely nothing. So it's not as simple as sticking this in visit and hoping for the best.

@raggi
Contributor
raggi commented Mar 7, 2012

Yes and no, visit is the critical one that everyone needs. It's possible we could also add a "server errors" method to the DSL, and users could poll that themselves.

Alternatively we could add checks throughout the rest of the DSL. In general, tests should fail either way, but it's the initial requests failing silently that frequently burn people.

@raggi
Contributor
raggi commented Mar 7, 2012

Ah, so.. I had a look at this.

It's easier than I thought, because of the way that DSL delegates to Session!

Thoughts on adding a check around every DSL delegate call?

@djanowski djanowski added a commit to djanowski/capybara that referenced this pull request Mar 16, 2012
@djanowski djanowski Raise errors from booted applications.
Thanks to @raggi (#660) for inspiration.
2dfa3c2
@raggi
Contributor
raggi commented Mar 30, 2012

ping to all, this would be useful to close up, happy to make any adjustments folks want.

@jonleighton
Contributor

+1 to this. It would be good if there was a way to also check errors when not using the DSL, but merging this for now seems good to me.

@jnicklas
Collaborator
jnicklas commented Apr 1, 2012

I still don't feel good about this. It feels too fragile to me. I'll ponder it for a little while longer.

@jonleighton
Contributor

Can you elaborate on what specifically seems fragile? Thanks

@jnicklas
Collaborator
jnicklas commented Apr 1, 2012

Just the whole approach of it. Catching errors, caching them somewhere, then at a random later point checking them and re-raising them. It just seems so wrong conceptually. I just feel very unenthusiastic about this. Maybe this is as good as we're going to get, but I'd rather spend some more time thinking about the problem than merging this without exhausting other options.

@jonleighton
Contributor

Well the fact that there are two threads of execution necessitates a synchronisation point.

Would you feel more comfortable if errors were checked on test failure? Something like "your test failed, we noticed there were some errors in the server thread, here they are: ..."

@djanowski
Contributor

@jonleighton: See #668 for a parallel discussion.

@raggi
Contributor
raggi commented Apr 5, 2012

@jnicklas having done this kind of work before, doing synchronization in a deterministic way is the only path to sanity. Further discussions over the alternative solutions are discussed on the other PR.

I do understand why you feel this is fragile, I'm afraid that adding the check in the consuming thread is a necessary requirement. Welcome to multithreaded programming.

EDIT: sorry, that sounds arrogant/condescending, I don't mean to be, it's just - this is how it's solved everywhere (when solved correctly (read: deterministically)).

@woahdae
woahdae commented Apr 6, 2012

Maybe we can just collect all errors that are raised, and then have a hook method which would allow us to re-raise those exceptions. Something like Capybara.current_session.reraise_captured_expressions! or something (the name is dumb). And we would call this method from hooks in capybara/rspec and capybara/cucumber. What do you think about that?

 

Would you feel more comfortable if errors were checked on test failure? Something like "your test failed, we noticed there were some errors in the server thread, here they are: ..."

This is more along the lines of what I would expect. I think you could handle all cases mentioned in the various tickets about this (ex. covering the case of testing error-handling code) if you collected exceptions, then in ex. rspec waited for a failure, say element not found, and had an output along the lines of "Element not found: blah\Exceptions encountered during run:\n [errors & traces]" This isn't as clean looking as stopping everything as soon as an error happens, but probably would have about the same effect since you typically can't progress your test after an error anyways. Note that if an error occurred but the test passed, no output would be expected. If there somehow were multiple errors, it would output all.

The downside is that you're still collecting errors, is that something you feel should also not need to be done?

@jnicklas
Collaborator
jnicklas commented Apr 6, 2012

@woahdae: that's more along the lines of what I had in mind.

@raggi
Contributor
raggi commented Apr 7, 2012

i disagree with this. Silently swallowing errors is horribly evil and also leads to hidden bugs.

@woahdae
woahdae commented Apr 7, 2012

If you're talking about something like foo rescue nil, then most definitely. On an integration level, however, what if I wanted to test that when an error happened, a notification was sent to the developers, and users were given a pretty error page? If the middleware were to stop the app on all exceptions, you would not be able to write such a test.

@jnicklas
Collaborator
jnicklas commented Apr 7, 2012

How is this "silently swallowing"? If anything it's "loudly swallowing". Nothing changes from the current behaviour except that we insert a middleware which simply prints out errors. That's it. I think that's a pretty reasonable approach.

@raggi
Contributor
raggi commented Apr 8, 2012

tests should fail if errors are happening

in a large and long running suite, stdout is not a reliable communication mechanism.

if anything so unfortunate is to be the final approach, please at least use stderr, but I am aggressively against a solution that fails to integrate with any testing practices or tooling.

@raggi
Contributor
raggi commented Apr 8, 2012

@woahdae the use case you describe does not raise an error out of the server

@woahdae
woahdae commented Apr 9, 2012

Hm, sorry for being dense, but would you mind unpacking that for me? If you handle an error with the default config.action_dispatch.show_exceptions = true or in a rescue_from, and the middleware catches the error before they execute and stops the test, then you won't be able to exercise that logic. If the middleware waits for errors after those get triggered, then it will never see errors unless you skip those methods. The only errors that would stop a test would be ones that were completely unhanded, which by default in testing is none (i.e. all errors are caught with a friendly 500 page by default). For example, the popular user access library CanCan works via raising and catching CanCan::AccessDenied errors; should the middleware fail a test on access denied? Obviously not, so it must sit after rescue_from, which means you must not handle any type of 500 in order to have tests fail in the manner you propose. As a specific example of where this would be a problem, my current client has a rescue_from that catches application errors in the context of xhr requests and returns a we're sorry js message as a failsafe so that users don't get confused when javascript triggers act strangely in error cases. I would need to disable this during test for your solution, no?

With the solution I/we just proposed, it's possible that even access denied errors could get output. If the test succeeds, no feedback needed; if it fails, it lets you know that, by the way, an access denied occurred. I'm not sure if this last detail is something that would have been planned, but it's the sort of thing that falls out of the general strategy.

Again, sorry if this is something obvious I'm missing, but I don't see how you can get the behavior you desire without the requirement of the app not handling any 500 errors in your integration environment. And as someone else pointed out, where do you draw the line? What about 404, 422, etc? From the perspective of a browser, all it knows is a status code, and that's not in itself indicative of failed application logic.

@jnicklas
Collaborator
jnicklas commented Apr 9, 2012

@woahdee: IIRC rescue_from is still run even if show_exceptions is false. We're talking about errors caught by the WebRick/Thin/whatever servers, which aren't bubbled.

@djanowski
Contributor

@raggi: I agree that stdout/err is not reliable and doesn't play well with other tools.

Question: with this approach, would errors triggered by interactions (like click_link) be raised correctly? Should we check for errors after delegating to the session?

All in all, I think we should try this approach first.

@woahdae
woahdae commented Apr 9, 2012

IIRC rescue_from is still run even if show_exceptions is false

Hm, didn't mean to imply otherwise... just to be clear, you're saying that all solutions require the error to hit webrick?

@raggi
Contributor
raggi commented Apr 13, 2012

@djanowski the server error would be raised first, which would prevent any interaction from actually occurring. This is essentially the same as if the error was raised by the server running synchronously as a callee of visit.

If you mean to ask about backtraces, those should be correct, so the output should not be confusing.

@s0meone s0meone added a commit to s0meone/capybara that referenced this pull request Jun 21, 2012
@raggi @s0meone raggi + s0meone merged pull request #660 69f3584
This was referenced Jul 9, 2012
@jnicklas jnicklas closed this in 6f145fb Jul 13, 2012
@jnicklas
Collaborator

I spent some time with this pull request, and there were a couple of things about it I didn't really like, so I went in a slightly different direction with it. Let me explain what I did:

  1. Moved the responsibility of starting the server to the session. As @raggi mentioned, this makes the implementation of this much easier, and it's a sensible thing to do anyway.

  2. Capture error raised inside the application.

  3. Re-raise those errors when Session#reset! is called.

The last point is obviously the controversial one, so let me explain why I did that. We obviously need some way of raising errors on the main thread, since, as @raggi explained, we would run into too many thread safety issues if we let the application thread raise those errors. So that leaves us two options:

  1. require the user to manually check for and raise exceptions
  2. raise exceptions automatically when some kind of user action occurs

This exception tried to go with option (2), but that has a huge problem, in my opinion: there are a huge amount of possible user actions in Capybara, and every single one of them would have to check for errors and raise them. Just jamming this in visit and only visit as this pull request did is confusing and insufficient. I considered going this route, but after realizing the sheer amount of methods I would have to add these checks to, I realized that it just isn't feasible without some crazy meta programming.

I chose to go with option (1) instead. At first I had a separate method, which I called raise_error!, but after spending some time with this, I realized that you always call reset_session! and raise_error! at the same time. There is no reason to call one without calling the other. Rolling this into reset_session! also gives us the added benefit that users of Capybara don't need to change anything about there code to benefit from this behaviour, no matter how they integrate Capybara into their workflow.

@matthijsgroen

Is there a way to opt-out from this behaviour? We use dummy image url that we just test the url of that they don't really are on our server is not important. (and not realistic considering we then have to include thousands of images in our codebase) right now we get routing errors where tests fail under capybara 2 and pass under capybara 1. This prevents us from upgrading and limits us in the versions of libraries to use. I couldn't find an option in the documentation, maybe I'm missing something.

@raggi
Contributor
raggi commented Jan 20, 2013

@matthijsgroen point the dummy image url somewhere other than your server...

@matthijsgroen

Yeah did that now, but also ajax calls that return 500 to be resolved in the front-end seem to trigger this behaviour. But from 40 failing scenario's after the upgrade I'm now down to 13, so there is progress.

@Systho
Systho commented Sep 26, 2013

Currently Capybara only reraises server error when Capybara::Session.reset!() is called which means at the end of the test. When using Capybara for user stories, one typically makes successive interactions (visit, then click here, click there, fill form, click button, then make some expectations ).

If the server raises an error during the last step, then the backtrace is printed as expected but if the server raises an error during an intermediate step then the only error displayed is one about a node not being present (either because of a failed expectation or a failed interaction)

This problem is hidden when the server and the browser are in the same thread since Capybara Middleware stores the error and immediately reraise it. But when the server is another process/thread then the browser is not able to see the error reraised by the Capybara::Server::Middleware . It would be great to check for server errors (5xx) after every browser interaction.

I made the following (horrible) monkey patch to workaround the problem but I don't feel confident enough to make a PR. This patch is really awful since it redefines every DSL methods instead of only the browser interaction but it did the trick for my use case

I hope it'll help to solve the problem (I'm currently using poltergeist as driver but I'm pretty confident the problem is the same for every drivers which cannot rescue the error raised by the Capybara::Server::Middleware )

module Capybara
  module DSLMonkeyErrorCheck

    ::Capybara::Session::DSL_METHODS.each do |method|
      define_method method do |*args, &block|
        res = page.send method, *args, &block
        raise page.server.error if Capybara.raise_server_errors and page.server and page.server.error
        res
      end
    end
  end
end
Capybara::DSL.send(:prepend, Capybara::DSLMonkeyErrorCheck)
@Systho Systho referenced this pull request in teampoltergeist/poltergeist Sep 26, 2013
Closed

Silent failure when pages return 500 responses #216

@raggi
Contributor
raggi commented Oct 13, 2013

My original patch did this, but it was rejected.

On Thu, Sep 26, 2013 at 6:18 AM, Systho notifications@github.com wrote:

Currently Capybara only reraises server error when
Capybara::Session.reset!() is called which means at the end of the test.
When using Capybara for user stories, one typically makes successive
interactions (visit, then click here, click there, fill form, click button,
then make some expectations ).

If the server raises an error during the last step, then the backtrace is
printed as expected but if the server raises an error during an
intermediate step then the only error displayed is one about a node not
being present (either because of a failed expectation or a failed
interaction)

This problem is hidden when the server and the browser are in the same
thread since Capybara Middleware stores the error and immediately reraise
it. But when the server is another process/thread then the browser is not
able to see the error reraised by the Capybara::Server::Middleware . It
would be great to check for server errors (5xx) after every browser
interaction.

I made the following (horrible) monkey patch to workaround the problem but
I don't feel confident enough to make a PR. This patch is really awful
since it redefines every DSL methods instead of only the browser
interaction but it did the trick for my use case

I hope it'll help to solve the problem (I'm currently using poltergeist as
driver but I'm pretty confident the problem is the same for every drivers
which cannot rescue the error raised by the Capybara::Server::Middleware )

module Capybara
module DSLMonkeyErrorCheck

::Capybara::Session::DSL_METHODS.each do |method|
  define_method method do |*args, &block|
    res = page.send method, *args, &block
    raise page.server.error if Capybara.raise_server_errors and page.server and page.server.error
    res
  end
end

end
end
Capybara::DSL.send(:prepend, Capybara::DSLMonkeyErrorCheck)


Reply to this email directly or view it on GitHubhttps://github.com/jnicklas/capybara/pull/660#issuecomment-25166456
.

@jnicklas
Collaborator

@raggi @Systho it wasn't so much about the idea than about the implementation. I wanted something which would have made the behaviour at least somewhat predictable, but I was unable to find a sane way to do it. I think you guys may be right though that having catching errors early is better. I've run into these situations as well. So I think I'm open to adding a check to all DSL methods (which is better, before or after?). This doesn't cover everything that can be done with Capybara, but it would make raise errors earlier in many cases, which is probably better than never, since I don't think we're going to get to always.

@Systho
Systho commented Oct 14, 2013

I think the rationale for catching the error early is that the error message is much more accurate than just having the following interaction or expectation crashing because some element is missing.

Therefore I would add a check after the interaction which might have failed instead of before the following interaction because the stacktrace would be misleading. If you chack for errors after interaction you can also limit the check to actions (click, visit, input manipulation, ...) and skip it for expectations.

@jnicklas
Collaborator

The problem with that reasoning is though that actions are often asynchronous, so the error may not have occured after the action is performed.

@Systho
Systho commented Oct 15, 2013

I didn't think of that problem.

Maybe the action methods could schedule check and the expectations methods could perform the check after the wait but before their job. That would enable the client to handle the server error instead of the "element missing" error and would not perform multiple checks when doing multiple assertions after a single interaction.

@raggi
Contributor
raggi commented Oct 28, 2013

@raggi @Systho it wasn't so much about the idea than about the implementation.

Sure. You may find it a good reference is all. It has the behavior you describe, but only hooks a single method - iirc. It was years ago now.

I wanted something which would have made the behaviour at least somewhat predictable, but I was unable to find a sane way to do it.

I'm pretty sure it is predictable, if all session access causes a check. It's much like the semantics of a private errno in that regard.

I think you guys may be right though that having catching errors early is better. I've run into these situations as well. So I think I'm open to adding a check to all DSL methods (which is better, before or after?).

Always before. The problem with after is that you can find the action causes more errors which will raise and take precedence, hiding the original error - in other words this falls back to todays harder to debug behavior. It's possible that before and after can lead to catching some more odd items, where the action causes an asynchronous error - but those are really really hard to make deterministic. As long as one can describe the behavior of when the check is done in a single sentence, and it is frequent, that's best.

This doesn't cover everything that can be done with Capybara, but it would make raise errors earlier in many cases, which is probably better than never, since I don't think we're going to get to always.

Nod.

@raggi
Contributor
raggi commented Oct 28, 2013

I think there's an important thing here, which is to focus on the right target. My target here is not to "have errors always appear in the correct place" - that's not possible, because there's no such thing as "the correct place" when crossing thread boundaries as this is. By contrast, what I want, is that errors are always raised before they cause a driver action to raise an only loosely related error. It is the latter that causes people to burn time into incorrect debugging, as in this case both the code position of the error and the message are invalid. We can fix one of these two items deterministically: the error message. We cannot fix the position, as already described, so making this aspect easier to understand is important. Hopefully this explains a little more why I say "before"

I can probably expand some more on how I've observed teams and individuals approaching debugging flows in these scenarios, and how they react to the various output scenarios, if that helps.

@jnicklas
Collaborator

The more I think about it, maybe we should somehow include this error check in the synchronize method. This way, it'll happen before almost everything you can do with Capybara, and we'll also raise the error if it happens after an action has been initiated. This would solve something like this:

click_link "Cause async error"
page.should have_content "Async action succeeded"

If Async action succeeded's check occurs right after Cause async action, we won't have caught the error in time, because it hasn't happened yet.

@zedtux zedtux referenced this pull request in bblimke/webmock Jan 9, 2014
Closed

Log WebMock::NetConnectNotAllowedError in Rails #353

@Systho
Systho commented Aug 3, 2015

I know this issue has been officially closed but at the same time it does not yet work as expected. @jnicklas do you have any idea on how you want to tackle this problem ? I think more and more app are using asynchronous drivers so I guess this problem will only amplify.

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