Problem/Motivation
@yash.rode is working on
š
Improve the user experience of having your staged update deleted before it was applied
Fixed
and got stuck on \Drupal\Tests\package_manager\Kernel\StageTest::testDestroyDuringApply() not working for several of the test cases. We spent an hour pairing to figure out the root cause. All he had done was at the end of \Drupal\package_manager\Stage::apply() add a new \Drupal\package_manager\Stage::setMetadata() call. There was no reason this should fail!
So he changed it to look like this:
public function apply(?int $timeout = 600): void {
ā¦ $event = new PreApplyEvent($this, $this->getIgnoredPaths());
$this->tempStore->set(self::TEMPSTORE_APPLY_TIME_KEY, $this->time->getRequestTime());
$this->dispatch($event, $this->setNotApplying());
// Create a marker file so that we can tell later on if the commit failed.
ā¦
try {
$this->committer->commit($stage_dir, $active_dir, $ignored_paths, NULL, $timeout);
}
catch (InvalidArgumentException | PreconditionException $e) {
ā¦
}
catch (\Throwable $throwable) {
ā¦
}
$this->failureMarker->clear();
$this->setMetadata('SOME KEY', 'SOME VALUE');
}
š Only that last line is new! And it's somehow causing test failures!
Why? Because setMetadata() calls ::checkOwnership() and ā¦ ownership had been lost by the time that last line was reached. But ā¦ how is that even possible? Ownership should only be lost if the stage was destroyed ā¦ oh but wait, the test method pretty much literally implies that. So let's look at that:
public function testDestroyDuringApply(string $event_class, bool $force, int $time_offset, bool $expect_exception): void {
$listener = function (StageEvent $event) use ($force, $time_offset): void {
// Simulate that a certain amount of time has passed since we started
// applying staged changes. After a point, it should be possible to
// destroy the stage even if it hasn't finished.
TestTime::$offset = $time_offset;
// No real-life event subscriber should try to destroy the stage while
// handling another event. The only reason we're doing it here is to
// simulate an attempt to destroy the stage while it's being applied, for
// testing purposes.
$event->getStage()->destroy($force);
};
$this->addEventTestListener($listener, $event_class, 0);
ā¦
Yep! The stage is being destroyed! Specifically, during the event that is specified, but that's provided by the data provider. The case he was debugging was the PreApplyEvent.
So: the test-owned listener would destroy the stage during PreApplyEvent ā¦ so how can we then crash literally on the last line of the apply() method?! Why did we even get to that point?! š¤Æš±
The next 20 minutes of debugging ensues and results in ā¦
ā¦ us realizing that immediately after dispatching the PreApplyEvernt, try { $this->committer->commit($stage_dir, $active_dir, $ignored_paths, NULL, $timeout); } happens, which is wrapped in a InvalidArgumentException | PreconditionException $e. Shouldn't ā¦ the stage dir actually existing ($event->getStage()->destroy($force); literally deletes that directory!) be a precondition that the committer would detect and shouldn't hence a PreconditionException have been thrown? š¤š¤š¤š¤š¤
Yes! \PhpTuf\ComposerStager\Infrastructure\Service\Precondition\StagingDirDoesNotExist will do exactly that! š
So we tried to figure out why, by stepping into the committer ā¦ and found it wasn't \PhpTuf\ComposerStager\Domain\Core\Committer\Committer but \Drupal\package_manager_bypass\Committer::commit, which does precisely zero precondition checking! ā ļø
The solution then was fairly simple: manually throw the appropriate PreconditionException.
ā ļø At this point, a very strong sense of fear and uncertainty overwhelms me: which test coverage can I still trust? š± 100% of the kernel tests use package_manager_bypass. (This makes sense; it's necessary for performance reasons, to be able to test many combinations of circumstances, which in functional tests would likely be too slow.)
Steps to reproduce
See above, do that on the code at commit 28b5b359691edd60e7e9a9c64f90980e31c8ff36.
Proposed resolution
Right now, \Drupal\package_manager_bypass\Committer \PhpTuf\ComposerStager\Domain\Core\Committer\Committer.
Same thing for \Drupal\package_manager_bypass\Beginner vs \PhpTuf\ComposerStager\Domain\Core\Beginner\Beginner.
And so on. None of them check any precondition.
\Drupal\Tests\package_manager\Kernel\StageTest::testDestroyDuringApply() and others seem to simulate only AU/PM-owned exceptions, without realizing that simulating what it takes to trigger some of those would also trigger PreconditionExceptions from the real php-tuf/composer-stager.
AFAIK (I can't know for sure because there are no docs š
) the purpose of package_manager_bypass is to avoid calling the actual composer executable, to "avoid testing the internet". +1 to that of course! But I think we've swing the pendulum too far to not using php-tuf/composer-stager at all in kernel tests instead of minimally using it.
If this requires upstream changes in php-tuf/composer-stager, then so be it. But we IMHO have to change this if we want to be able to trust our tests. Every project using php-tuf/composer-stager with solid test coverage will have a need to avoid calls to the composer executable, so I think it'd be a reasonable addition to php-tuf/composer-stager.
Or ā¦ was this very approach recommended by the php-tuf/composer-stager maintainer @TravisCarden? š¤
Remaining tasks
TBD
User interface changes
None.
API changes
None.
Data model changes
None.
