Add functional test that proves there is reasonable UX whenever Composer Stager operations have a hard failure

omkar.podey → made their first commit to this issue’s fork.

Isn't this very similar to \Drupal\Tests\automatic_updates\Functional\UpdateFailedTest

re #3 fixed the summary

yes it is similar but in \Drupal\Tests\automatic_updates\Functional\UpdateFailedTest the failure happen in the Composer Stager "Commit" stage. So therefore have to assume the active codebase has a problem because there was problem copying from the stage to the active directory. That is why we create the failure marker fail.

In this proposed test the exception should be come from NoOpStager::setException(fixed in summary). In that case the active code base is unchanged so there is no need for the failure marker file.

re #7 updated the "Proposed solution" to be clearer

There are just 2 small nits other than that the test looks clear to me!

Reviewing

Looks good to me! 💯💯

At a glance, I don't think this is complete.

For one thing, we also need coverage of what happens when the Beginner and Committer services throw. We don't necessarily need to do that with a data provider, if it's easier; we could have separate test methods for those. But the Beginner, the Stager, and the Committer are the three Composer-centric services we call during the update, and we need to know how we handle an exception from each of them.

Having said all that, I do see that the issue summary didn't mention the Beginner and Committer. In my view, that's an oversight.

I'll add that.

Yep i do agree we should test other services too.

+1 for that. The current state of the MR looks great already though! 👍

I think this looks very promising. It could use some cleanup and explanation in certain areas, though...

Nits fixed.

Note that for the one question I had, which @omkar.podey redirected to @tedbow, I figured out the answer.

I think the changes in that hunk look good. But they're not strictly necessary: tests pass without any of the changes to BatchProcessor. So I leave it to @tedbow and @phenaproxima to decide whether to merge this with or without that hunk.

Related: IMHO 📌 Failure marker message does not contain exception message + backtrace if available Fixed should be required to update the test coverage this introduces to prove that the appropriate stack traces are displayed if and only if error_level == verbose.

Finally, based on @tedbow's comment at https://git.drupalcode.org/project/automatic_updates/-/merge_requests/83... and him having written the code that I observed in #26 to not be required for the tests to pass … makes me think it should be @tedbow who first confirms the RTBC and then @phenaproxima to do a final pass before committing it.

This looks really good. A few easy suggestions to increase clarity, though...

Actually, reassigning to @tedbow for review, as per #28.

@tedbow has a lot of stuff on his review plate. Let's get @omkar.podey to address your feedback first. In #28 I just asked for @phenaproxima to review first and confirm RTBC: he effectively did that in #29.

So, let's get it 100% ready for @tedbow to commit :)

Just 2 small suggestions otherwise I think this looks good.

I found three nitpicks and one request for a comment to prevent confusion later. But otherwise, I agree with Ted, this is pretty much RTBC.

Still needs a comment to prevent confusion about the second StageFixtureManipulator call.

One very last thing: a single missing comment! 😄

I like it!

Once we have the build test failure resolved, this is good to go, in my opinion.

Automatically closed - issue fixed for 2 weeks with no activity.