Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.We had some warnings in #2659806: Warnings when adding an action which I assume should be thrown during the ConfigureAndExecuteTest as well. If so, we need to fix our tests to check for such warnings.
If the warnings aren't thrown in our tests, we should instead improve our test coverage.
Comments
Comment #2
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedComment #3
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedComment #4
Cogax CreditAttribution: Cogax commentedI think the error is caused by
AddExpressionForm::validateFormwhich calls ::submitForm of the expression form handler. But the first step of the form has it's own submission handler, which is ignored. I'll try to fix that. It's the same forConditionFormas well asActionForm.https://github.com/fago/rules/pull/380
Comment #5
klausiThe PR belongs into #2659806: Warnings when adding an action, this issue is about improving our browser test base class.
Comment #6
klausiMy idea for this: https://github.com/fago/rules/pull/382
I know, this should all be in core BrowserTestBase, but let's add it in Rules for faster testing and iteration.
Comment #7
klausiYay, tests failing on the PHP warnings as expected!
Another idea would be to modify the Guzzle client if the mink GoutteDriver is used with http://docs.guzzlephp.org/en/latest/handlers-and-middleware.html
That way we could really cover all responses received and would not need to add the processHeaderErrors() method everywhere. The disadvantage is that it then only works for the GoutteDriver, but maybe that is ok?
Comment #8
fagoI'd dislike doing it in a certain driver only, as we loose the flexibility to run our tests with another driver what might be really important in future to cover JS stuff.
Thus, reading up on how to do it best with Mink generically brought me to:
https://github.com/jhedstrom/drupalextension/issues/143
and the conclusion: It cannot work as there might be ajax requests etc. which are not controlled by mink at all.
As a consequence, I'd suggest doing it different by running a manual check at the end of each test which checks the Drupal watchdog table for new entries. I know this is not easy as we do not have a Drupal booted up and we cannot rely on drush for that, so maybe we could just add in the drupal-driver project and use it's API driver to use regular Drupal APIs & ensure there are no watchdog entries?
https://github.com/jhedstrom/DrupalDriver
I guess that would imply a Drupal bootstrap also, quite unfortunate :(
Other idea:
- Let's add a small test helper module, which adds a separate route for verifying that there are no watchdog entries. Then, install the test helper module for all browser tests and call the route / http resource at the end of each test to verify watchdog all is fine?
- In the end this also a drupal bootstrap, just elsewhere ;-) One in the test context might be cheaper - behat / drupalextension does it as well and this seems to work fine & fest in general.
Comment #10
klausiHm, that sounds all way to complicated IMO.
Drupal core already sends us the PHP error assertions in the HTTP response headers, we just need to read them.
I committed my pull request as a stop gap for now so that we catch PHP errors with the current tests at least.
Setting to "needs work" for future ideas and improvements.
Comment #11
TR CreditAttribution: TR commentedI don't see anything here that still needs to be done.