drupalextension icon indicating copy to clipboard operation
drupalextension copied to clipboard

Handling unexpected Drupal errors and warnings

Open jonathanjfshaw opened this issue 8 years ago • 6 comments

It seems to me that in most circumstances step authors would want the actions described in steps to flow successfully as described, without displaying Drupal errors or warnings on the page (that is, Drupal user-facing error and warning messages, not the PHP errors and warnings rejected in #143). It would seem a solid design principle to allow errors and warnings only when explicitly mentioned in the scenario, rather than allowing them to slide silently by unnoticed.

I therefore suggest we have an ErrorsandWarningsContext. If this context is used in a suite, then a step in that suite fails if it results in a Drupal error or warning on the page and the scenario or feature does not have an @hasDrupalError or @hasDrupalWarning tag.

Creating such a context is not particularly hard, but it requires a knowledge of uncommon things like getDrupalSelector, @afterStep hooks, and especially because of the problems with scenario tags addressed in #347 and my comment on that. Therefore it might be helpful to offer the context in this extension.

A side benefit of using such a context is that it facilitates debugging. Problems that generate unexpected Drupal errors or warnings often lead to exceptions in a subsequent step; this approach catches the problem closer to the root and provides a more informative error.

I have code ready to contribute if the idea is interesting.

jonathanjfshaw avatar Feb 04 '17 18:02 jonathanjfshaw

This sounds like a good idea for 4.x. I like that it will by default assume that there will be no errors or warnings in any scenario, but allows to opt out if you specifically want to test for error messages in a scenario. We can't add this to 3.x, because it might cause unexpected failures if people are running current tests that have warnings or errors, that is a B/C break.

What do you think of calling the tags @ignoreDrupalErrors and @ignoreDrupalWarnings?

We already have the MessageContext class which would be an ideal place for this functionality.

pfrenssen avatar Apr 04 '17 10:04 pfrenssen

@ignoreDrupalErrors and @ignoreDrupalWarnings

Sure.

We can't add this to 3.x, because it might cause unexpected failures if people are running current tests that have warnings or errors, that is a B/C break.

I have it as a separate context for that reason. The other advantage of a separate context is that it enables people to opt out globally (by not using the context). But on balance I think you're right, anyone who wants it turned off globally is doing something unforseeably weird, let's put it in MessageContext in order to keep the overall number of contexts manageable.

jonathanjfshaw avatar Apr 04 '17 11:04 jonathanjfshaw

This is currently blocked on https://github.com/Behat/MinkExtension/issues/285. It seems like scenarios are not properly isolated in the mink extension, as the current path carries over from one to the next. Because of that, looking for on-page errors on every step is problematic, as if you haven't visited a page yet in this scenario, Mink thinks you're on a page left over from the previous scenario and will detect on-page errors from that.

jonathanjfshaw avatar Jul 31 '17 13:07 jonathanjfshaw

I really like this idea. I think it's something we tried to do way back before this was even its own project for testing Drupal.org itself, but ran into issues. I wonder if this new context could somehow keep track itself if its in a new scenario or not...that sounds pretty messy though. I'm guessing https://github.com/Behat/MinkExtension/issues/285#issuecomment-322408963 doesn't help?

jhedstrom avatar Dec 19 '17 00:12 jhedstrom

I hoped someone upstream would help as it seems a major upstream bug, but no luck. I've not tried the further debugging suggested.

I think the easy solution for us is to manually reset the path to NULL in an @beforeScenarioHook. This should get this context working.

Shall I create a PR of my code for such a context using such a hook?

jonathanjfshaw avatar Dec 19 '17 09:12 jonathanjfshaw

@jonathanjfshaw sure, that sounds like a reasonable workaround. It could have an @todo pointing to the upstream issue too so we don't forget about it :)

jhedstrom avatar Dec 19 '17 17:12 jhedstrom