Fix flaky test: testInstallUnlockMessage

I was able to consistently reproduce this issue locally about every third or fourth test run with

ddev phpunit --filter=testInstallUnlockMessage

When I deleted the `__timestamp` key/value, then it would consistently pass. I believe it's because of the ::getFirstUpdatedTime() method call in the ::begin method. Of course, I could be completely wrong.

Can you explain a bit more about why the __timestamp thing makes it fail? It's hard for me to grok how that's related, or why it would make the test fail only now and then, rather than every time.

To be honest, I'm not 100% sure... I started stepping through the InstallerController::begin() method and saw this line:

$updated_time = $this->installState->getFirstUpdatedTime();

Which is returning the `__timestamp` value from the key/value store:

return $this->keyValue->get('__timestamp');

The only place I found the `__timestamp` value being set was in InstallState::setState(). In this method, it is being set if not exists.

$this->keyValue->setIfNotExists('__timestamp', $this->time->getRequestTime());

So, my theory was that the `__timestamp` value was not getting cleared/reset during the test's call to ::setFakeTimeByOffset(), and causing the flaky behavior.

Could it be that $this->keyValue->setIfNotExists should actually be $this->keyValue->set()?

Yeah, this is a tricky one. I think we should probably figure out exactly what's causing the flakiness, so that we don't introduce possible fragility (or worse, more flakiness) by implementing a fix even we don't understand :) That said, the tracing of it to __timestamp is a really helpful lead.

Maybe I was over thinking it. If we're reliant on time later in the tests, we should probably mock the initial request time. This seems to be giving me consistent passing tests locally.

I am still getting the failure locally, but it happens less frequently now. One out of every ten or fifteen times now. I think that request time needs to be set to a constant instead of relying on the modifier, which puts us at the mercy of the container's time.

There's something else going on here, but I'm not sure what it is.

It's not that the time needs to be constant; TestTime is already able to override the request time, and it does it in a way that works. The problem is that it doesn't work consistently. I suspect this has something to do with the reliance on state, which is a destructable cache collector.

So I also suspect container wonkiness, but until I completely understand why, I'd rather hold off and take the time to figure out the real cause, rather than just try to find any fix that seems to work.

We should increase the scope of this one.

The unlock behavior in InstallerController is a hard-to-parse mess, and is the only part of our tests that necessitates a date-time decorator. We could remove that (and move the time override into InstallerController specifically), which should sidestep this issue. While we're at it, we should refactor the unlock logic to make more sense, have better phrasing, and remove crufty messages that are never presented to the user.

This does introduce a behavior change, but I think that's worth the increased clarity. Users don't care about "1 hour, 15 minutes"; "an hour ago" is sufficient, and line with what you see elsewhere on the internet.

Self-assigning to ensure the flakiness is squashed.

I pushed a commit that ran the faulty test 100 times. It passed every run in four different jobs:

• https://git.drupalcode.org/issue/project_browser-3506178/-/jobs/4463800
• https://git.drupalcode.org/issue/project_browser-3506178/-/jobs/4463808
• https://git.drupalcode.org/issue/project_browser-3506178/-/jobs/4463804
• https://git.drupalcode.org/issue/project_browser-3506178/-/jobs/4463796

400 straight passes? I think we can safely say the flakiness is eliminated. State caching turned out to be the problem.

Just needs final review and an issue summary update.

Looks good to me!

More than anything on this one, the comments are the real MVP.

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