Problem/Motivation

#3320782: xdebug being enabled causes tests to fail without clear indication that it is the problem we found the if the xdebug validator failed then \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testCron would fail at $assert_session->pageTextContains('Your site is ready for automatic updates.');

Basically this check if meant to confirm that there no status check validation errors or warning for Automatic Updates.
But the message is not very helpful because it does not tell you what error is on the page.

in #3320782 we will solve this xdebug but in general if a status check has error or warning this be displayed in the test failure

Steps to reproduce

To see the difference this merge you have to get a validator to fail. We will force 1 for example but problems in your local set-up or a logic error in our build tests could cause this to happen.

Run these steps in first 8.x-2.x then the merge request branch

  1. Cause ComposerSettingsValidator to fail by changing

    if ($config->get('secure-http') !== TRUE) {
    to
    if ($config->get('secure-http') !== FALSE) {

  2. Run \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testCron

In 8.x-2.x the failure should be

Behat\Mink\Exception\ResponseTextException : The text "Your site is ready for automatic updates." was not found anywhere in the text of the current page.

in the merge request branch the error should be

Unexpected status check output on the status report page:
HTTPS must be enabled for Composer downloads. See the Composer documentation for more information.

Simply searching for "HTTPS must be enabled for Composer downloads" will find you the validator that is failing. "Your site is ready for automatic updates." not being on your page tells you either, the report didn't load at all because of some error or it did load and there was some error with some unknown validator.

With the same change to ComposerSettingsValidator running \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::testUi will result in this error

Behat\Mink\Exception\ElementNotFoundException : Button with id|name|title|alt|value "Update to 9.8.1" not found.
/Users/ted.bowman/sites/au-contrib/vendor/behat/mink/src/Element/TraversableElement.php:112
/Users/ted.bowman/sites/au-contrib/modules/automatic_updates/tests/src/Build/CoreUpdateTest.php:112

while the merge request again would fail with

Unexpected status check output on the status report page:
HTTPS must be enabled for Composer downloads. See the Composer documentation for more information.

Proposed resolution

Actually display the status check errors

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

Issue summary: View changes
tedbow’s picture

Status: Active » Needs review
tedbow’s picture

Issue tags: +sprint
wim leers’s picture

Status: Needs review » Needs work
Issue tags: +DX (Developer Experience)

3 remarks on the MR wrt test strength.

tedbow’s picture

Assigned: Unassigned » traviscarden
traviscarden’s picture

Assigned: traviscarden » Unassigned

@tedbow, per our offline conversation, I agree that this issue doesn't require my expertise, specifically. Unassigning myself.

kunal.sachdev’s picture

Assigned: Unassigned » kunal.sachdev
kunal.sachdev’s picture

Assigned: kunal.sachdev » Unassigned
Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work

Sorry, that fix is inadequate I'm afraid 😅 See comment on the MR.

yash.rode’s picture

Assigned: Unassigned » yash.rode
yash.rode’s picture

Assigned: yash.rode » Unassigned
wim leers’s picture

#3320782: xdebug being enabled causes tests to fail without clear indication that it is the problem landed.

And seeing this in relation to #3319679: Assert known preconditions for test runs and fail early if unmet made me realize that based on the issue title, the current MR does not yet do what we need it to do: this really needs to update \Drupal\Tests\package_manager\Traits\AssertPreconditionsTrait, which would then also achieve what the current MR achieves, but it'd make it apply to all tests!

This is actually a blocker for #3319679: Assert known preconditions for test runs and fail early if unmet!

tedbow’s picture

re #14 I don't think belongs in \Drupal\Tests\package_manager\Traits\AssertPreconditionsTrait because

  1. Status check on the status report page only applies to Automatic Updates not package manager
  2. it doesn't apply to kernel or build unit tests
  3. that trait uses setUpBeforeClass so it can assert conditions at the earliest possible point, hence preconditions. You can't run the status checks or that status report page at this point. Other times we don't sign with just minimal permissions necessary to do the test task which does not including assessing the status report page.
  4. many test we set up the tests to have problems with particular status checks. Often there will be errors in status checks before the call to \Drupal\Tests\automatic_updates\Functional\AutomaticUpdatesFunctionalTestBase::mockActiveCoreVersion and we don't call this in setup because different test methods need to mock different versions
tedbow’s picture

Title: Fail early in Automatic Updates build tests if status report has related errors/warnings » Fail early in Automatic Updates build and functional tests if status report has related errors/warnings
wim leers’s picture

Status check on the status report page only applies to Automatic Updates not package manager

🤯

But what about \Drupal\package_manager\Validator\ComposerPatchesValidator, \Drupal\package_manager\Validator\ComposerSettingsValidator, \Drupal\package_manager\Validator\ComposerJsonExistsValidator et cetera?

Even if currently it is exposed to A) end users in the UI, B) if AU is installed and not just PB … I think we should still run all known validators as a precondition, otherwise you'll end up debugging tests unnecessarily?

In fact, that'd have saved me half a day of work over at #3331168: Limit trusted Composer plugins to a known list, allow user to add more

wim leers’s picture

Title: Fail early in Automatic Updates build and functional tests if status report has related errors/warnings » Fail early in Automatic Updates build and functional tests if StatusCheckEvent subscribers fail

Updating title to reflect what I'm proposing in #17 — which would remove the reliance on the UI 😊

Curious about your thoughts, @tedbow!

tedbow’s picture

Assigned: Unassigned » wim leers

I think we should look at the suggestion in #17 but I think we should do the original intent of this was just provide this check for build tests. Because the build test has to rely on the UI unless we build more test infrastructure like `\Drupal\package_manager_test_event_logger\EventSubscriber\EventLogSubscriber::logEventInfo` but for running the status checks, otherwise a build test doesn't have another way to know if the status checks pass.

So I would suggest

  1. revert back to https://git.drupalcode.org/project/automatic_updates/-/merge_requests/58... the last commit that I pushed that was still only for build tests. I had tried to address @Wim Leers feedback on that approach. Review and commit that approach
  2. Open another issue to add an assertStatusCheckSuccessful() that would used in kernel and functional test. On that issue if we decide if that is something we can run for all these tests or if it would only make sense to call sometimes(most of the and classes have to opt out?) because certain test methods are setup specifically to fail these checks
  3. in 2) decide if it makes sense to remove the UI approach in build tests in favor of calling our assert in a test module in build tests.

I think we should do 1) now because build tests are the hardest to debug and this would help a lot, even it is the final way we handle this

wim leers’s picture

wim leers’s picture

Assigned: wim leers » tedbow

I thought, but I was wrong!

The scope of this issue is to check StatusCheckEvent before starting the whole build process test (or at least as early as possible).

In #3331168: Limit trusted Composer plugins to a known list, allow user to add more, I did improve build test coverage relating to validation and in principle also around StatusCheckEvent. But:

  • it aimed to improve the DX of build tests by not failing in obfuscated ways, by surfacing the actual error messages on the visited pages during the build test ⇒ during, not before/early
  • it aimed to do so for all events, not just status check events

Opening a new issue…

wim leers’s picture

Title: Fail early in Automatic Updates build and functional tests if StatusCheckEvent subscribers fail » Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers) + make functional tests fail earlier
Assigned: tedbow » wim leers

Hah, just discussed with @tedbow, and he actually wants to expand the scope of this issue slightly because there is a lot of overlap in the code needed for the current scope and what I did over there 😄🎉

Bringing over the relevant pieces from #3331168: Limit trusted Composer plugins to a known list, allow user to add more in a new MR.

wim leers’s picture

The new branch was ~2 months behind because that's when the issue fork was created … so first caught it up to current 8.x-2.x.

wim leers’s picture

Brought over the 4 essential commits from #3331168: Limit trusted Composer plugins to a known list, allow user to add more. Tests pass locally 👍

But … ModuleUpdateTest fails locally. I was hoping to only have to modify that test in #3331168: Limit trusted Composer plugins to a known list, allow user to add more, but it looks like the improved error checking in Build tests show that this is actually pseudo-broken in HEAD — HEAD just happens to ignore the error message 😅

Let's see if DrupalCI confirms that finding …

wim leers’s picture

Looks like ModuleUpdateTest passes on 10.0.x. But I'm fairly certain it'll fail on 9.5.x. Queued test for that. See #3331168-17: Limit trusted Composer plugins to a known list, allow user to add more for the analysis behind that statement.

wim leers’s picture

Assigned: wim leers » tedbow

Well … okay … I don't understand this, but for some reason it's passing. Okay. Yay for a smaller/simpler set of changes here then! 😄

As discussed, back to you, @tedbow, to extract part of what I did and run it automatically at the start of each build test! 😊

(Then @tedbow can review what I did so far, and I'll review @tedbow's subsequent commits. 👍)

tedbow’s picture

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

Ok brought in my changes into 3320792-explicit-early .

1 test failed but I pushed up a fix for it

tedbow’s picture

Created #3337049: Assert no errors after creating the test project in ModuleUpdateTest as a follow up but that issue is tagged contrib-only so I am not adding the sprint tag since we don't need to do it now.

tedbow’s picture

Title: Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers) + make functional tests fail earlier » Make build tests fail 1) more explicitly, 2) earlier when possible (failing StatusCheckEvent subscribers)
Related issues: +#3337054: Assert status checks pass as early as possible in kernel and functional tests

I would really like to get this in and I don't think the functional test are going to share that much code so I rescoping this the orignal

We could use the same code in functional test like I did in the original MR https://git.drupalcode.org/issue/automatic_updates-3320792/-/blob/332079...

but I think that is overkill for the functional tests to rely on the UI when I think all we have to do in functional test is to use our existing function like
$this->assertStatusCheckResults([]);

We just have to figure where to call because some functional test are step up to specifically fail status checks and other don't enable automatic_updates until later so calling
$this->assertStatusCheckResults([]); would only assert package_manager validators

and we can't simply enable automatic updates , $this->assertStatusCheckResults([]); and then disable it because some test what happens when we enable automatic_updates so doing this would change test coverage.

TLDR I don't want to figure all that out right now and put off the fix for build tests.

Created follow-up #3337054: Assert status checks pass as early as possible in kernel and functional tests

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

Let's ship this! 🚢

This is definitely a net improvement, and all your changes make sense to me 👍

tedbow’s picture

Assigned: Unassigned » tedbow

`composer phpcs` ran locally

merged 8.x-2.x , will commit when green

  • 0316b695 committed on 8.x-2.x
    Issue #3320792 by tedbow, Wim Leers, kunal.sachdev: Make build tests...
tedbow’s picture

Status: Reviewed & tested by the community » Fixed

🎉

Status: Fixed » Closed (fixed)

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