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

Command icon 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:

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Version: 8.x-2.x-dev » 3.0.x-dev
tedbow’s picture

Issue summary: View changes
Issue tags: +sprint
tedbow’s picture

Issue tags: -sprint
tedbow’s picture

Issue tags: +sprint
wim leers’s picture

💯, this will likely allow me to close that other issue at this point! 🎉

(Typing this from my phone, will find it later.)

yash.rode’s picture

Assigned: Unassigned » yash.rode
Status: Active » Needs work
wim leers’s picture

phenaproxima made their first commit to this issue’s fork.

phenaproxima’s picture

OK, 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 UnknownFilesExcluderTest which DOES have a relocated docroot. It should set that up simply by overriding createTestProject() and moving the files around, much as is already done in setUpFilesystem(). 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.

phenaproxima’s picture

Assigned: yash.rode » phenaproxima

I know what's breaking these tests, so self-assigning to resolve those.

wim leers’s picture

Therefore, I suggest we do the following:

Split UnknownFilesExcluderTest into two tests: […]

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.

That'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! 😄

phenaproxima’s picture

Assigned: phenaproxima » wim leers
Status: Needs work » Needs review

I think this will pass, so assigning to Wim for review.

phenaproxima’s picture

Assigned: wim leers » phenaproxima
Status: Needs review » Needs work

Given 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...

phenaproxima’s picture

Status: Needs work » Postponed

I 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.

wim leers’s picture

Assigned: phenaproxima » Unassigned
Status: Postponed » Closed (outdated)

I 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:

$ git log --oneline --since 2023-01-23 | wc -l
     171
git d b485deb6 414dd683
…
 344 files changed, 12984 insertions(+), 10245 deletions(-)
$ git d b485deb6 414dd683 -- package_manager/src/Validator
 package_manager/src/Validator/AllowedScaffoldPackagesValidator.php    |  80 ++++++++++++++++++++++
 package_manager/src/Validator/BaseRequirementValidatorTrait.php       |  50 ++++++++++++++
 package_manager/src/Validator/BaseRequirementsFulfilledValidator.php  |  71 +++++++++++++++++++
 package_manager/src/Validator/ComposerExecutableValidator.php         | 208 -------------------------------------------------------
 package_manager/src/Validator/ComposerJsonExistsValidator.php         |  75 --------------------
 package_manager/src/Validator/ComposerMinimumStabilityValidator.php   |  90 ++++++++++++++++++++++++
 package_manager/src/Validator/ComposerPatchesValidator.php            | 165 +++++++++++++++++++++++++++++++++++++++-----
 package_manager/src/Validator/ComposerPluginsValidator.php            | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 package_manager/src/Validator/ComposerSettingsValidator.php           |  66 ------------------
 package_manager/src/Validator/ComposerValidator.php                   | 143 ++++++++++++++++++++++++++++++++++++++
 package_manager/src/Validator/DiskSpaceValidator.php                  |  35 ++--------
 package_manager/src/Validator/DuplicateInfoFileValidator.php          |  84 +++++++++--------------
 package_manager/src/Validator/EnabledExtensionsValidator.php          |  88 ++++++++++++++++++++++++
 package_manager/src/Validator/EnvironmentSupportValidator.php         |  28 ++++----
 package_manager/src/Validator/LockFileValidator.php                   |  70 ++++++++-----------
 package_manager/src/Validator/MultisiteValidator.php                  |  47 ++++---------
 package_manager/src/Validator/OverwriteExistingPackagesValidator.php  |  77 ++++++++-------------
 package_manager/src/Validator/PendingUpdatesValidator.php             |  40 +++--------
 package_manager/src/Validator/PhpExtensionsValidator.php              | 103 ++++++++++++++++++++++++++++
 package_manager/src/Validator/PhpTufValidator.php                     | 180 ++++++++++++++++++++++++++++++++++++++++++++++++
 package_manager/src/Validator/PreOperationStageValidatorInterface.php |  25 -------
 package_manager/src/Validator/SettingsValidator.php                   |  21 ++----
 package_manager/src/Validator/StageNotInActiveValidator.php           |  34 ++++-----
 package_manager/src/Validator/StagedDBUpdateValidator.php             |  67 ++++++------------
 package_manager/src/Validator/SupportedReleaseValidator.php           |  36 +++++++---
 package_manager/src/Validator/SymlinkValidator.php                    | 122 ++++++++++-----------------------
 package_manager/src/Validator/WritableFileSystemValidator.php         |  40 ++---------
 package_manager/src/Validator/XdebugValidator.php                     |  85 -----------------------
 28 files changed, 1431 insertions(+), 939 deletions(-)

and perhaps most crucially:

 package_manager/tests/src/Kernel/AllowedScaffoldPackagesValidatorTest.php   |  61 +++++++++
 package_manager/tests/src/Kernel/BaseRequirementsFulfilledValidatorTest.php |  93 ++++++++++++++
 package_manager/tests/src/Kernel/ComposerMinimumStabilityValidatorTest.php  |  54 ++++++++
 package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php           | 304 ++++++++++++++++++++++++++++++++++++-------
 package_manager/tests/src/Kernel/ComposerPluginsValidatorTest.php           | 405 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 package_manager/tests/src/Kernel/ComposerValidatorTest.php                  | 173 +++++++++++++++++++++++++
 package_manager/tests/src/Kernel/DiskSpaceValidatorTest.php                 |  11 +-
 package_manager/tests/src/Kernel/DuplicateInfoFileValidatorTest.php         |  64 +++++-----
 package_manager/tests/src/Kernel/EnabledExtensionsValidatorTest.php         | 163 +++++++++++++++++++++++
 package_manager/tests/src/Kernel/EnvironmentSupportValidatorTest.php        |  10 +-
 package_manager/tests/src/Kernel/LockFileValidatorTest.php                  |  69 +++++++---
 package_manager/tests/src/Kernel/MultisiteValidatorTest.php                 |  69 ++++++----
 package_manager/tests/src/Kernel/OverwriteExistingPackagesValidatorTest.php | 130 +++++++++++--------
 package_manager/tests/src/Kernel/PendingUpdatesValidatorTest.php            |   8 +-
 package_manager/tests/src/Kernel/PhpExtensionsValidatorTest.php             |  77 +++++++++++
 package_manager/tests/src/Kernel/PhpTufValidatorTest.php                    | 250 ++++++++++++++++++++++++++++++++++++
 package_manager/tests/src/Kernel/StagedDBUpdateValidatorTest.php            |   7 +-
 package_manager/tests/src/Kernel/SupportedReleaseValidatorTest.php          |  36 ++----
 package_manager/tests/src/Kernel/SymlinkValidatorTest.php                   | 277 +++++++++++++++++++++++----------------
 package_manager/tests/src/Kernel/WritableFileSystemValidatorTest.php        |  11 +-
 20 files changed, 1949 insertions(+), 323 deletions(-)

So … I'm going to mark this Closed (outdated).

tedbow’s picture

I agree this is outdated. Thanks for the work on it though