Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
In #3338666: Add functional test that proves there is reasonable UX whenever a stage event subscriber has an exception, we added a new test method to UpdateErrorTest, called testExceptionFromEventSubscriber(). This is a very thorough test method that walks through the entire attended update process, asserting that exceptions thrown by subscribers to all of the stage events are caught, logged, and redirected properly.
The existing methods of UpdateErrorTest have some amount of overlap with this new test, and that means the test is longer and possibly more confusing than it needs to be, as @tedbow detailed in #3338666-47: Add functional test that proves there is reasonable UX whenever a stage event subscriber has an exception.
Proposed resolution
Let's tighten up UpdateErrorTest as a whole, and ensure that there's no duplicate coverage. That will means refactoring or removing some methods entirely, and adding more assertions to testExceptionFromEventSubscriber() -- for example, to assert that the expected buttons are present (or not present), and that we're on the pages we expect to be at certain points.
Issue fork automatic_updates-3354325
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
phenaproximaComment #3
phenaproximaComment #4
kunal.sachdev commentedComment #6
phenaproximaAfter discussion with @kunal.sachdev, I'm gonna take this on to try and clarify it. It's very complicated, touchy stuff.
Comment #7
phenaproximaI think this makes sense. Assigning to Wim for review.
I also think Kunal's instincts are right on the money; the PostRequireEvent check is indeed redundant, and was never reachable because of the early return. That's okay; because it's redundant, I removed it.
Comment #8
wim leers🏓
Comment #9
phenaproximaComment #10
wim leersThis was tricky to review, and in doing so, I think I found a now misnamed test method, one piece of now superfluous test logic and one missing assertion.
Assigning to @phenaproxima to confirm.
Comment #11
phenaproximaBack to you, Wim.
Comment #12
wim leersThis looks far better now, and with excellent clarifications from @phenaproxima that really get to the heart of the problem: what UX the user is forced to go through depending on:
👍
🚢
Comment #13
wim leersOh, and per https://git.drupalcode.org/project/automatic_updates/-/merge_requests/84..., @phenaproxima, you are saying that the current test coverage proves that #3346644: If an error occurs during an attended process, delete the stage on behalf of the user is unnecessary because it already works that way, right?
Comment #14
phenaproximaThat issue is still necessary, because currently the update process does not delete the stage if any event has a failure. It does it for some, but not others. The test coverage here proves that.
PostCreateEvent errors will not destroy the stage; the other first-phase events will. I don't think a failure in any of the second-phase events will destroy it. That issue exists to decide if and how to make the behavior consistent.
Comment #16
phenaproximaComment #17
wim leersThen I'm confused why we removed the
@todo😇 But no big deal — we still have the issue. 👍Comment #18
phenaproximaWe didn't.
The todo is still there, just a bit higher. (It was already there in two places.) We removed the irrelevant reference to it, but kept the relevant one.
Comment #19
tedbowComment #20
wim leers#18 Oh, I must've missed that then!