Problem/Motivation
Follow-up to #3320792: Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers)
This is for functional and kernel tests
Tagging this as core-post-mvp because I don't think the other test are in as bad a shape as build test were before #3320792: Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers).
They are much faster and general giver better feedback if you ValidationResut error you were expecting, especially since we got \Drupal\Tests\package_manager\Traits\ValidationTestTrait::assertValidationResultsEqual
which will show you in the error message of the results
Steps to reproduce
Proposed resolution
Determine if in \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::setUp and/or \Drupal\Tests\automatic_updates\Functional\AutomaticUpdatesFunctionalTestBase::setUp
We could just run $this->assertStatusCheckResults([]); because expect everything to be ok at this point
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork automatic_updates-3337054
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:
- alternate
changes, plain diff MR !779
- 3337054-assert-status-checks
changes, plain diff MR !771
Comments
Comment #2
tedbowComment #3
tedbowComment #4
tedbowComment #5
tedbowComment #6
wim leers💯, this will likely allow me to close that other issue at this point! 🎉
(Typing this from my phone, will find it later.)
Comment #7
yash.rode commentedComment #8
wim leersAs promised in #6.
Comment #12
phenaproximaOK, so! I spent an hour going over this today with @yash.rode, so I could understand the problem thoroughly. And now I do.
I see why this has been so knotty to untangle - it's hard to get in and do the alterations we need to do, at the time we need to do them. But, I have pushed a solution that is related, but slightly different. I'd much prefer we DID NOT override
setUpFilesystem()for all Package Manager kernel tests -- changing something that low-level, that broadly, is asking for trouble. Especially when the problem is really arising from a tightly scoped conflict between SiteFilesExcluder and certain scenarios in UnknownPathExcluderTest.Therefore, I suggest we do the following:
Split UnknownFilesExcluderTest into two tests: UnknownFilesExcluderTest, which doesn't have a relocated docroot (or, if you prefer, "nested web root"), and
UnknownFilesExcluderWithRelocatedDocrootTest extends UnknownFilesExcluderTestwhich DOES have a relocated docroot. It should set that up simply by overridingcreateTestProject()and moving the files around, much as is already done insetUpFilesystem(). Then, the data provider in UnknownFilesExcluderTest can be slimmed down; it only needs four cases, and the same cases will be tested in a relocated docroot by the subclass.Comment #13
phenaproximaI know what's breaking these tests, so self-assigning to resolve those.
Comment #14
wim leersThat's exactly what I suggested and what I was asking @yash.rode to do 😅
But doing it in
createTestProject()is too late based on my analysis late last week (when I first paired for a solid while with @yash.rode to get him on track to fixing this), because the kernel will already have booted at that point, and that then makes excluders think the project root is somewhere else than it actually is.Looking forward to seeing my understanding getting proven wrong though! 😄
Comment #15
phenaproximaI think this will pass, so assigning to Wim for review.
Comment #17
phenaproximaGiven some of the clean-up I've been able to do here, I'm concerned about UnknownFilesExcluder. Either it isn't working, or our coverage is fatally flawed. Need to examine more closely...
Comment #18
phenaproximaI think I need to take a closer look at this and refactor that test to be...like, understandable. It's WAY too complicated right now to make head or tail out of. That, for the record, is a pre-existing condition in HEAD.
That said, this is post-MVP, so I have to deprioritize it for now. I'm leaving this assigned to myself, since I have my head wrapped around it pretty clearly.
Comment #19
wim leersI think 90% of the complexity here could be removed if we would execute
$this->assertStatusCheckResults([]);in a different kernel. But that too is non-trivial. But at least it would avoid changing code all over the place just for the sake of avoiding static caching that is otherwise totally reasonable.This was opened at a time where we had lots of productivity/velocity issues due to absence of validators, less strict validators, and we especially did not yet have the concept of base requirement validators (introduced in #3312960: Create an API for base requirement validators which run before other validators and stop event propagation if they fail).
Package Manager & Automatic updates are wildly different today compared to 3.5 months ago:
and perhaps most crucially:
So … I'm going to mark this .
Comment #20
tedbowI agree this is outdated. Thanks for the work on it though