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
- Cause ComposerSettingsValidator to fail by changing
if ($config->get('secure-http') !== TRUE) {
to
if ($config->get('secure-http') !== FALSE) { - 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
Issue fork automatic_updates-3320792
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
Comment #3
tedbowComment #4
tedbowComment #5
tedbowComment #6
wim leers3 remarks on the MR wrt test strength.
Comment #7
tedbowComment #8
traviscarden commented@tedbow, per our offline conversation, I agree that this issue doesn't require my expertise, specifically. Unassigning myself.
Comment #9
kunal.sachdev commentedComment #10
kunal.sachdev commentedComment #11
wim leersSorry, that fix is inadequate I'm afraid 😅 See comment on the MR.
Comment #12
yash.rode commentedComment #13
yash.rode commentedComment #14
wim leers#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!
Comment #15
tedbowre #14 I don't think belongs in
\Drupal\Tests\package_manager\Traits\AssertPreconditionsTraitbecausesetUpBeforeClassso 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.Comment #16
tedbowComment #17
wim leers🤯
But what about
\Drupal\package_manager\Validator\ComposerPatchesValidator,\Drupal\package_manager\Validator\ComposerSettingsValidator,\Drupal\package_manager\Validator\ComposerJsonExistsValidatoret 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…
Comment #18
wim leersUpdating title to reflect what I'm proposing in #17 — which would remove the reliance on the UI 😊
Curious about your thoughts, @tedbow!
Comment #19
tedbowI 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
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 checksI 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
Comment #20
wim leersActually, I think I already wrote everything that we need here … I was forced to do so in #3331168: Limit trusted Composer plugins to a known list, allow user to add more.
See #3331168-13: Limit trusted Composer plugins to a known list, allow user to add more through #3331168-17: Limit trusted Composer plugins to a known list, allow user to add more and all commits + comments in between.
Verifying that now…
Comment #21
wim leersI thought, but I was wrong!
The scope of this issue is to check
StatusCheckEventbefore 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:Opening a new issue…
Comment #22
wim leersHah, 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.
Comment #24
wim leersThe 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.Comment #25
wim leersBrought over the 4 essential commits from #3331168: Limit trusted Composer plugins to a known list, allow user to add more. Tests pass locally 👍
But …
ModuleUpdateTestfails 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 inBuildtests 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 …
Comment #26
wim leersLooks like
ModuleUpdateTestpasses on10.0.x. But I'm fairly certain it'll fail on9.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.Comment #27
wim leersWell … 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. 👍)
Comment #28
tedbowOk brought in my changes into 3320792-explicit-early .
1 test failed but I pushed up a fix for it
Comment #29
tedbowCreated #3337049: Assert no errors after creating the test project in ModuleUpdateTest as a follow up but that issue is tagged
contrib-onlyso I am not adding thesprinttag since we don't need to do it now.Comment #30
tedbowI 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 validatorsand 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
Comment #31
wim leersLet's ship this! 🚢
This is definitely a net improvement, and all your changes make sense to me 👍
Comment #32
tedbow`composer phpcs` ran locally
merged 8.x-2.x , will commit when green
Comment #35
tedbow🎉