#13 - the proposal is not to change all instances everywhere, it is to allow either. Issues to switch from one to another should be closed won't fix unless the maintainers actively want to switch.
Currently someone adding image styles would already need to add the convert action, so I guess this setting would just provide the default format at that point? Or would we look to at the conversion to all image styles? Also, not all image styles have dimensions so I guess the source filesize could be useful there
One more set of tests:
No fix with 1/500 fails - https://git.drupalcode.org/issue/drupal-3519621/-/jobs/4992774
Fix branch with 0/1000 fails - https://git.drupalcode.org/issue/drupal-3519621/-/jobs/4992773
Another run with a fail before the runner died https://git.drupalcode.org/issue/drupal-3519621/-/jobs/4992772
And a run with the fix and 0 fails https://git.drupalcode.org/issue/drupal-3519621/-/jobs/4992730
This is a pretty simple one. In the test we load a page with an iframe and immediately start making assertions about it. But the content of the iframe takes some time to load. Most of the time it's very quick, but if we inject some artificial slowness in to \Drupal\media\Controller\OEmbedIframeController::render then we can reliably reproduce it. On my local it is sufficient to add usleep(50000);.
Here's the failing job (2/500) - https://git.drupalcode.org/issue/drupal-3519621/-/jobs/4992589
Pushing up a fix
Opened 🐛 [random test failure] MediaSourceOEmbedVideoTest::testMediaOEmbedVideoSource Active as I ran in to it yesterday and it was previously reported in #188.
mstrelan → created an issue.
Submitted a review but it seems to be the same stuff @longwave already suggested and was apparently fixed, so maybe a rebase went wrong?
Yes but we're talking about whether to convert to AVIF or webp, not whether to run the image style at all.
Right, same thing. In most cases we already know the dimensions of the output. So if the image style is for a small thumbnail, just enable the convert to webp plugin. If it's for a hero image then enable the convert with fallback plugin (or just convert to avif if you know its supported). Point is, the source image could be very large or very small but the output dimensions are the same.
I've attached an example in a zip since we can't upload webp or avif here. Original image is 7.4 MB. When I upload it to the media library we get a thumbnail by default. As a WEBP that's 3.3 KB but if I convert to AVIF it's 2.6 KB. That would fall under the "not worth the CPU cycles" threshold, but since the original is 7.4 MB we convert it anyway.
Some more conversions at 📌 [PP-1] Convert functional tests in navigation module to kernel tests Postponed .
I was able to work around more issues with multiple drupalGet calls, this time with lazy builders. The issue was that we needed to reset the MemoryBackend cache after the first drupalGet. This is something we already do in UiHelperTest by calling $this->refreshVariables. Here is the commit in that issue in case we want to add it here.
But I ran in to another issue - installing modules doesn't reset the hook implementations. So in NavigationDefaultBlockDefinitionTest::testNavigationDefaultBeforeNavigation where it tries to install navigation_test_block first, and navigation later, the hook_page_top implementation in navigation module is never invoked. I'm not sure a way to solve that yet.
I don't think we can generate two outputs to compare and pick one to use. The URL to the file is generated before the file is generated and as catch pointed out they would have different file extensions.
I would have though the threshold would be based on the file size of the original image (e.g. what @andypost says in #6), so wondering why you think it would be useless?
If your image style is a 16x16 pixel thumbnail and your original image is 100MB the JPG thumbnail will still be very small.
I would suggest adding to the new AvifConvert plugin. What is the logic for the threshold, is it the original file size? File size after converting (tricky)? Dimensions? The first one is fairly useless, the second one I think impossible since the derivative won't exist first time, and third one you can just opt not to use the convert plugin on the image style.
mstrelan → created an issue.
I don't think \Drupal\Tests\Composer\Plugin\Scaffold\Fixtures::createIsolatedComposerCacheDir actually works. It creates the dir and sets the env var, but composer doesn't actually use it.
I verified this by putting a breakpoint after the composer require command and inspecting the tmp dir and seeing it's empty. Then I added the following command before composer require and when I step through it the cache gets populated.
$this->mustExec("composer config cache-dir " . getenv('COMPOSER_CACHE_DIR'), $sut);.
So if we want to pre-populate the cache then we need to ensure the cache dir actually works.
You're right, it did, see https://git.drupalcode.org/issue/drupal-3354138/-/jobs/4978417/viewer
I suppose we could run it a few more times to see if it's consistently in that realm, but it's pretty slow (48 mins for 500 runs). Then we'd need to run the fix that many times as well.
Had a look in to this and not sure it is entirely accurate anymore. I started by disabling wifi and running ScaffoldTest, yet it passed every time. Possibly there is a composer cache somewhere, but it passed even after clearing my local composer cache. Regardless, the test apparently creates an isolated cache for each test run.
Then I tried ScaffoldUpgradeTest, but it is currently skipped because 8.8.0 doesn't support Composer 2. If I unskip it and bump the requirement to 9.5.0 it then complains that the path repo with 11.x-dev has higher priority. I've managed to work around that and have opened 📌 Unskip ScaffoldUpgradeTest Active to fix it. With wifi disabled I then get "Could not resolve host: repo.packagist.org", and once I enable wifi the test passes.
So we need to verify that ScaffoldTest is indeed accessing packagist (maybe the path repo is taking priority now) and we need a solution for ScaffoldUpgradeTest when repo.packagist.org is down. Maybe we can set up a mock mirror or we can prepopulate the cache from a fixture.
mstrelan → created an issue.
I used the fancy new REPEAT_COUNT var to run the new version 1000 times - https://git.drupalcode.org/issue/drupal-3393137/-/jobs/4978074
mstrelan → changed the visibility of the branch 3496319-multiple-run-as-is to hidden.
mstrelan → changed the visibility of the branch 3496319-multiple-runs-key-value to hidden.
mstrelan → made their first commit to this issue’s fork.
mstrelan → changed the visibility of the branch 3393137-repeat to hidden.
Fine by me, restoring status from #10 since nothing has changed to review.
I don't think we're gonna catch this in the repeating test job. Should have snuck it in to #3041318: [random test failure] Various fails when a random string is used in an xpath selector → but too late now. MR with the fix is up.
Ran it 7000 times without the fix and couldn't get it to fail.
1000x https://git.drupalcode.org/issue/drupal-3393137/-/jobs/4978095
1000x https://git.drupalcode.org/issue/drupal-3393137/-/jobs/4978103
5000x https://git.drupalcode.org/issue/drupal-3393137/-/jobs/4978177
Maybe it's less frequent now or doesn't happen at all. Guess we just leave this until we see it in the wild again?
Upstream PR is merged, just need a release then we can bump php-tuf/composer-stager. Or maybe we postpone again on
📌
Update Composer dependencies for 11.2.0
Active
.
Do we even need this function? It's only used when throwing this exception:
throw new \RuntimeException(sprintf('%s registered its own error handler (%s) without restoring the previous one before or during tear down. This can cause unpredictable test results. Ensure the test cleans up after itself.',
$this->name(),
self::getCallableName($handler),
));
Wouldn't it be enough without self::getCallableName($handler)?
This looks great and is so much simpler.
For example, go to https://git.drupalcode.org/issue/drupal-3519177/-/pipelines/473447, click on Repeat Test Class then enter the variables.
Yes please. While we're at it, can we add a var for number of repeats? It's currently hardcoded at --repeat "100". Would be good if we could set this var via the UI like REPEAT_TEST_CLASS.
We can do a separate issue for that, or we can re-title / re-purpose this issue to include this.
We'll probably need to split this in to 5 separate issues, but adding all to one MR for now.
mstrelan → created an issue.
Rebased the MR and fixed all the deprecated calls. Have also updated the CR.
I guess when we remove $message in 12.x we'll need to deprecate passing NULL as the third param, and then only allow a string in 13.x
Non-exhaustive review. Added some thoughts to the MR. I tend to find enums most useful when you can pass them around to functions rather than just relying on them for their backed values. In an ideal would you could use the enum for field config as well rather than the int value.
Apologies, the MR that had the random fail was older than 🐛 [random test failure] FilterEntityReferenceTest Active and did not have the fix.
Opened 🐛 [random test failure] FilterEntityReferenceTest (again) Active as this test can still randomly fail
mstrelan → created an issue.
Core issue commited, probably the issue could be moved to core
✨ AVIF conversion with WEBP fallback Active is open against core now.
Actually the Convert plugin in core already supports AVIF if the GD toolkit supports it. This issue in the AVIF contrib module is for providing the same conversion without GD, e.g. via ImageMagick or CAVIF.
Switched to keyvalue instead of state
There are two sides to this, both encoding and decoding. I think encoding is less important, as it only happens once. It's possible that over time decoders will become more efficient, e.g. by using AV1 hardware decoding instead of software decoding.
A few links
Sniffs were not discussed here. What, if any, changes are required in Coder?
We don't need to change anything to allow the proposed changes, only if we want to restrict them.
I tested this by updating the @param doc on a constructor param to use a variety of different types listed in the phpstan docs. I also started putting random strings in, and they mostly passed, unless I used any of @#,.. For example, @param zDSA[]|<>D-&* $date_formatter is acceptable, but @param zDSA[]|<>D-&*# $date_formatter is not.
Moved the query inside the wait callback.
Updated the IS, hope that helps.
This is pretty hard to debug without knowing what $block_selector, $block_plugin or $new_page_text is. Either way, the block selector element is becoming visible (we're asserting that) but disappearing by the time we try to click it. But if sleeping for 100ms fixes it then I guess the element is eventually being replaced again. Maybe there is something real we can fix to prevent that, but otherwise we probably just need to wait until the element is really ready to be clicked. We could put the whole element find / mouseover / click in to a waitFor callback as a bandaid.
@quietone the problem is more hypothetical and the update here is for best practice. I'll try to explain the problem.
- In
\Drupal\system\Controller\DbUpdateController::helpfulLinkswe check if the user has the'administer site configuration'permission before displaying the "Status report" link. - In
\Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testSuccessfulMultilingualUpdateFunctionalitywe create a user with a bunch of permissions. This includes both'access site reports'and'administer site configuration'. Then we assert that the "Status report" link exists. - Core later decides that the permission required to access the
'system.status'route should actually be'access site reports'and not'administer site configuration'so the route is updated. The test still passes because that user has both permissions. But a user with'administer site configuration'and not'access site reports'will still see the link, even though they won't be able to access the route. - The same problem could arise if a contrib or custom module decides to alter the permissions required for the route. For example, they might override the status report page and provide a custom permission for it. The link will still appear for users with the original permission.
By changing to Url::access we don't need to worry about what permission the route requires, just whether the logged in user can access it or not.
Can we add a test for this?
I think the test mentioned above is sufficient, not sure it's worth the effort to expand that further.
Updated title and issue summary, hope that's clearer
Added explanation inline. It can check up to 10 times within the 100ms timeout, but that assumes it takes no time at all to check.
Yep and I think we should leave it for now until we find a need for it.
smustgrave → credited mstrelan → .
Can you explain a bit more about where this path appending thing happens? Is it something we can maybe fix?
I'm offline for over a week now but when from what I recall you can see it in the html output for ExperimentalHelpTest if you remove the slash and run the test. Didn't look at what was causing it.
Why would the block report as being broken? What's causing it to be broken? This test needs to explain what it is doing!
IIRC there is a test module that includes config for a block, but that block is referencing the uuid of a block content entity that doesn't exist. Couldn't tell you why, but it's not really relevant. I think let's ignore that test for now.
Added 2 more, HelpTest and HelpTopicTest, both of this are much faster now but I don't have the numbers. I did have a go at HelpTopicsSyntaxTest but it was actually significantly slower!
@dww sure, I've created an MR against 11.x. As a bonus you don't have to review it one commit at a time because all the changes are limited to the help module, other than a7f43433.
Added a comment in #3517299-4: [PP-1] Convert functional tests in help module to kernel tests → that the only thing blocking multiple requests in the tests I've looked at is the missing preceding slash.
UiHelperTrait has a buildUrl method that ensures the path is absolute, I think we need to do the same thing here.
I investigated the multiple requests issue a little further, and it turns out the only issue is that the paths weren't prefixed with a preceding slash, so the path was appended to the path of the previous request resulting in 404s. After fixing those the updated timing is below.
MySQL
ExperimentalHelpTest - Kernel 3.246s, Functional 13.550s
HelpBlockTest - Kernel 2.818s, Functional 13.642s
HelpPageOrderTest - Kernel 3.216s, Functional 14.067s
HelpPageReverseOrderTest - Kernel 3.212s, Functional 13.407s
NoHelpTest - Kernel 3.831s, Functional 15.771s
Total: Kernel 16.323s, Functional 70.437s
MySQL w/ tmpfs
ExperimentalHelpTest - Kernel 1.739s, Functional 6.374s
HelpBlockTest - Kernel 1.634s, Functional 5.254s
HelpPageOrderTest - Kernel 1.686s, Functional 5.254s
HelpPageReverseOrderTest - Kernel 1.700s, Functional 5.404s
NoHelpTest - Kernel 1.754s, Functional 5.754s
Total: Kernel 8.513s, Functional 28.04s
Those numbers make this much more compelling.
Opened 📌 [PP-1] Convert functional tests in help module to kernel tests Postponed with a few more help tests I converted, see https://git.drupalcode.org/issue/drupal-3517299/-/merge_requests/1/diffs for the diff.
Added an MR against 3390193-add-drupalget-with-mink-to-kerneltestbase but since it's against that branch it doesn't appear on the issue page, and the pipelines don't work. But I did it this way so we can see only the changes to Help module and not all the changes in the parent issue.
https://git.drupalcode.org/issue/drupal-3517299/-/merge_requests/1
It was a little annoying having to deal with the fact that we can't perform multiple requests in the same test, but overall it's still faster for the cases that I split. My test times below:
MySQL
ExperimentalHelpTest - Kernel 6.348s, Functional 13.550s
HelpBlockTest - Kernel 8.376s, Functional 13.642s
HelpPageOrderTest - Kernel 3.216s, Functional 14.067s
HelpPageReverseOrderTest - Kernel 3.212s, Functional 13.407s
NoHelpTest - Kernel 7.468s, Functional 15.771s
Total: Kernel 28.62s, Functional 70.437s
MySQL w/ tmpfs
ExperimentalHelpTest - Kernel 3.396s, Functional 6.374s
HelpBlockTest - Kernel 4.992s, Functional 5.254s
HelpPageOrderTest - Kernel 1.686s, Functional 5.254s
HelpPageReverseOrderTest - Kernel 1.700s, Functional 5.404s
NoHelpTest - Kernel 3.574s, Functional 5.754s
Total: Kernel 15.348, Functional 28.04s
So for MySQL with a normal filesystem this is a massive improvement! For tmpfs it's a decent improvement for the tests that only had one drupalGet, but it's much less of an improvement when the test had to be split and we repeat the setup tasks. It might be worth waiting for 📌 Allows DrupalKernel::handle to handle more than one requests (rough support for PHP-PM) Needs work for those tests.
mstrelan → changed the visibility of the branch 3390193-add-drupalget-with-mink-to-kerneltestbase to hidden.
mstrelan → created an issue.
I'm not sure whether to:
a. call AssertContentTrait::setRawContent in drupalGet()
b. change getTextContent() to use Mink, since that's now available? And then we can later on deprecate setRawContent().
I'm not sure either. I don't think we can deprecate setRawContent because I think some tests use it after directly rendering things not using drupalGet. Note that UiHelperTrait::drupalGet has this comment that I think is not true either, because setRawContent only happens in KernelTestBase.
* @return string
* The retrieved HTML string, also available as $this->getRawContent()
@mstrelan could you try adding this to the top of AssertContentTrait::getTextContent()?
Yeah that worked. I think probably the simplest solution is to call $this->content = $out; from the new drupalGet, that also makes it work.
Could you push your work on that test somewhere so I can have a look at it?
Added patches for POC I did on block_content and help tests.
The docs on HttpKernelUiHelperTrait::drupalGet() say that page caching modules won't work. Should that list of warnings be moved from the method docs to the trait docs, so they're more visible?
I must admit I didn't look at the docs, I just expected it would work like BrowserTestBase. I don't think moving them would make a difference. I do think we could probably work around this though by either replacing that policy or modifying it to use a service that can be replaced or decorated to pretend it's not the CLI. But this is best suited to a follow up.
The 50x run has passed 3 times in a row now:
https://git.drupalcode.org/issue/drupal-3497701/-/jobs/4854823
https://git.drupalcode.org/issue/drupal-3497701/-/jobs/4856362
https://git.drupalcode.org/issue/drupal-3497701/-/jobs/4864839
Postponing on https://github.com/php-tuf/composer-stager/pull/414 but we could also focus on speeding up the test instead of increasing the timeout.
I tried to convert a few BrowserTestBase tests to kernel tests to try this out and have a couple observations.
1. \Drupal\Tests\help\Functional\HelpPageOrderTest
This calls $this->getTextContent() which in BrowserTestBase resolves to \Drupal\Tests\UiHelperTrait::getTextContent, but in KernelTestBase resolves to \Drupal\KernelTests\AssertContentTrait::getTextContent which depends on AssertContentTrait::setRawContent being called. Of course the test could be refactored to not call $this->getTextContent(), but I'm wondering if we should do something about that here, such as calling AssertContentTrait::setRawContent in drupalGet. Once I worked around it the Kernel test passed in 3 seconds as opposed to 12 seconds before.
2. \Drupal\Tests\block_content\Functional\BlockContentPageViewTest
This required lots of refactoring, which is as expected, but I couldn't get the foobar_gorilla block to load in ::testPageEdit. I was able to manually place the system_powered_by_block and get that to load, so I'm willing to concede that there's probably something I'm missing, rather than blocks not appearing because of the new drupalGet.
3. \Drupal\Tests\dynamic_page_cache\Functional\DynamicPageCacheIntegrationTest
This was probably a bit ambitious. The first blocker I couldn't easily work around was that the default cache policy for DPC prevents caching if PHP_SAPI is cli, which it is now that we're in a Kernel test. This might lead to other gotchas down the track if folks are expecting DPC to work when using drupalGet in Kernel tests.
That's as much as I've got time for today. Unfortunately I didn't find an existing test that could be converted cleanly, but nonetheless I'm quite excited about this.
I feel a bit uneasy about the test case. The existing tests run through a number of valid and invalid scenarios for the default validator. But currently you're replacing the default validator, so now we don't know if the scenarios would pass or fail with the original validator.
I think instead we should have an additional test case that is explicitly to test that swapping out the validator is respected by ::validateProps.
Honestly I don't know why we need to be testing the behaviour of the codeblock plugin. This is provided by CKE5 core and is thoroughly tested there. We should only be testing that we can configure the languages, and that the languages get passed to the plugin. I'd argue we don't need a JS test at all for this, but I guess a basic smoke test that the plugin is loading and the configured languages are available would be reasonable.
Came across this triaging issues for Bug Smash Initiative and have a few thoughts:
- Since we're moving to Navigation module I checked if that is also affected and it is
- Since Claro and Starterkit Theme are unaffected should we really be fixing it for Stark?
- As per bnjmnm a stylesheet is certainly better than a style attribute. Not just for customisations, but for Content Security Policy also.
- I don't think toolbar.module is the right component for this since the issue is really with the dialog. Probably Javascript or Stark would be better. Even if it's a css fix, it's a javascript component we're styling.
Converted to an MR, updated line numbers in the IS.
Fixed \Drupal\Tests\views\Kernel\Plugin\ExposedFormRenderTest::testExposedFormRawInput.
mstrelan → made their first commit to this issue’s fork.
Is this still relevant since
✨
Use one-time login link instead of user login form in BrowserTestBase tests
Fixed
? In most cases the test no longer requires the login form, unless the $useOneTimeLoginLinks property is false. Furthermore, the report in the issue summary suggests you're using
Drupal Test Traits →
, so you could always override the drupalLogin method in your own test base class.
Thanks @mondrake, some great points there. My initial thoughts on those:
1. I think automatically applying this rule everywhere in contrib and custom code would be quite disruptive, so it's almost a nice side effect that it would be opt-in. Ultimately I don't really care where it lives. It could probably even be a phpcs sniff that can be auto-fixed with phpcbf.
2. I think that could be handled separately in a follow up? Better to make progress than to get it perfect first go. Unless doing this makes things worse than the current state.
3. Absolutely. Keen to get more feedback first though.
Managed to fix 405 files and only had one fail. Have added a @phpstan-ignore to NodeAccessGrantsCacheContextTest for now. Changing from a Plan to a Task and setting Needs Review.
The IS mentions both Kernel and Functional tests should use \Drupal::service, but according to alexpott in #2699565-27: Replace \Drupal:: with $this->container->get() in test classes that allow dependency injection of Basic Auth module → this is only true for Functional tests.
I've created an MR with a phpstan rule to identify functional tests that access $this->container. See https://git.drupalcode.org/issue/drupal-2066993/-/jobs/4841841 for results. We could land this and add the existing violations to the baseline. Then we can open follow up issues to cleanup the baseline. I suspect it should be simple enough to write a rector rule to replace $this->container->get() with \Drupal::service().
mstrelan → made their first commit to this issue’s fork.
I think this is great, do we need something more generic long term?
Yeah the attribute I mentioned in the proposed resolution would be good to investigate. We can probably make use of the native phpunit attribtues for tests that are skipped on requirements like php extensions. Might be good to see if we can do another attribute for skipping on certain database driver too. But long term any permanently skipped tests should be fixed :joy:
mstrelan → created an issue.
Now we have yet-another-timeout to investigate - installing standard profile times out after 300 seconds:
Package Update (Drupal\Tests\package_manager\Build\PackageUpdate)
✘ Package update
┐
├ Symfony\Component\Process\Exception\ProcessTimedOutException: The process "/usr/local/bin/php ./core/scripts/drupal install standard --no-ansi" exceeded the timeout of 300 seconds.
│
│ /builds/issue/drupal-3497701/vendor/symfony/process/Process.php:1182
│ /builds/issue/drupal-3497701/vendor/symfony/process/Process.php:451
│ /builds/issue/drupal-3497701/vendor/symfony/process/Process.php:252
│ /builds/issue/drupal-3497701/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php:336
│ /builds/issue/drupal-3497701/core/tests/Drupal/BuildTests/QuickStart/QuickStartTestBase.php:40
│ /builds/issue/drupal-3497701/core/modules/package_manager/tests/src/Build/TemplateProjectTestBase.php:187
│ /builds/issue/drupal-3497701/core/modules/package_manager/tests/src/Build/TemplateProjectTestBase.php:334
│ /builds/issue/drupal-3497701/core/modules/package_manager/tests/src/Build/PackageUpdateTest.php:22
┴
This failed for me on local the second time I ran it. I set up a job to repeat it 100 times and it failed 5/100 times, see https://git.drupalcode.org/issue/drupal-3389365/-/jobs/4840855
mstrelan → made their first commit to this issue’s fork.
The plot thickens. Looks like package manager has a bunch of special handling around composer patches. I've managed to run a test locally that will patch php-tuf/composer-stager to pass the timeout through to the rsync command. I tested it my setting the timeout to 1 second in ApiController::createAndApplyStage and it failed after 1 second instead of 120.
I have a repeat job running with the timeout set to 180 - https://git.drupalcode.org/issue/drupal-3497701/-/jobs/4832276. If that passes (maybe we run it a few times), then we can probably postpone this on https://github.com/php-tuf/composer-stager/pull/414
Is there a way we can refactor the core MR to do this? E.g. maybe CoreServiceProvider passes a container param with some hints of where to scan, the custom module can modify the container param and then the BundleClassCollectorPass can find them.
Wondering if this can be incorporated in to the core MR which now does its own discovery outside of plugins - https://git.drupalcode.org/project/drupal/-/merge_requests/11545
In its current state it searches for an Entity subdir in all container.namespaces. Would be worth seeing if it can be made to work with this use case.
I think this is by design:
* This function loads the body part of a partial HTML document and returns a
* full \DOMDocument object that represents this document.
and
* @param string $html
* The partial HTML snippet to load. Invalid markup will be corrected on
* import.
Both suggest it's only intended to deal with partial snippets, not full documents.
Pushed an update to assert the response content, and restore the previous execution time to see if that catches anything.
The 120 timeout is still curious. I can't get xdebug working inside the php local server so it's hard to step through. Looks like it would be passing 300 all the way down to \PhpTuf\ComposerStager\Internal\FileSyncer\Service\FileSyncer::sync which calls $this->environment->setTimeLimit($timeout); but I'm not sure that actually affects anything, e.g. in \PhpTuf\ComposerStager\Internal\FileSyncer\Service\FileSyncer::runCommand the timeout is not passed to $this->rsync->run so it looks like it would just fallback to 120.
1. We should try to optimise the test and/or package_manager so we don't need a 30 second timeout.
I must admit I haven't tried to grok everything this test is trying to achieve, but it feels like PackageUpdateTest::testPackageUpdate could use a mocked response from the controller, and the controller could be tested independently. But maybe that's losing end-to-end coverage that we're trying to test for.
2. We should try to make sure that fatal errors result in a 500 instead of 200 so we don't get false negatives on 200 assertions.
Another approach would be to return a success string from the controller and assert that we get that in the response. That would catch the OOM and PHP timeout issues we were seeing locally.
https://git.drupalcode.org/project/drupal/-/jobs/4824652 is the repeat test class job running with that change, it's varying between 400-700 seconds to complete, so very slow indeed, but so far each iteration is passing.
That job passed 96 times and failed 4 times. This time instead of there being no error response we have an error to look at which I've included below, I've replaced html encoded entities for readability. The TL;DR is the rsync command is sometimes timing out after 2 minutes. This timeout seems to come from \PhpTuf\ComposerStager\API\Process\Service\ProcessInterface::DEFAULT_TIMEOUT, although it looks like \Drupal\package_manager\StageBase::create should be passing its own default, which is 300, so I'm not sure what's going on there.
Error response: The website encountered an unexpected error. Try again later.Drupal\package_manager\Exception\StageException: Failed to run process: <em class="placeholder">The process "'/usr/bin/rsync' '--archive' '--checksum' '--delete-after' '--verbose' '--exclude=/PACKAGE_MANAGER_FAILURE.yml' '--exclude=/web/sites/simpletest' '--exclude=/vendor/.htaccess' '--exclude=/web/sites/default/files' '--exclude=/web/sites/default/files/.sqlite' '--exclude=/web/sites/default/files/.sqlite-shm' '--exclude=/web/sites/default/files/.sqlite-wal' '--exclude=/recipes' '--exclude=/web/sites/default/default.settings.php' '--exclude=/web/sites/default/default.services.yml' '--exclude=/web/sites/default/settings.php' '--exclude=/web/sites/default/settings.local.php' '--exclude=/web/sites/default/services.yml' '--exclude=/package_manager_test_event.log' '--exclude=/tmp/.package_managerd1e3013e-0986-48a6-993d-25ac3459c1b2/ccKD8PwqcDLB4Q2kf2hMBv1EFnJulEs6' 'tmp/build_workspace_29b4565cc0c9b8a68abd3d8a6d0e7b8fGPvmzn/project/' 'tmp/.package_managerd1e3013e-0986-48a6-993d-25ac3459c1b2/ccKD8PwqcDLB4Q2kf2hMBv1EFnJulEs6'"; exceeded the timeout of 120 seconds.</em> in Drupal\package_manager\StageBase->rethrowAsStageException() (line 371 of core/modules/package_manager/src/StageBase.php). Symfony\Component\Process\Process->wait() (Line: 252)
├ Symfony\Component\Process\Process->run(Object, Array) (Line: 269)
├ Symfony\Component\Process\Process->mustRun(Object) (Line: 105)
├ PhpTuf\ComposerStager\Internal\Process\Service\Process->mustRun(Object) (Line: 81)
├ PhpTuf\ComposerStager\Internal\Process\Service\AbstractProcessRunner->run(Array, Object, Array, Object) (Line: 121)
├ PhpTuf\ComposerStager\Internal\FileSyncer\Service\FileSyncer->runCommand(Object, Object, Object, Object) (Line: 58)
├ PhpTuf\ComposerStager\Internal\FileSyncer\Service\FileSyncer->sync(Object, Object, Object, Object, 300) (Line: 38)
├ PhpTuf\ComposerStager\Internal\Core\Beginner->begin(Object, Object, Object, Object, 300) (Line: 43)
├ Drupal\package_manager\LoggingBeginner->begin(Object, Object, Object, Object, 300) (Line: 354)
├ Drupal\package_manager\StageBase->create() (Line: 111)
├ Drupal\package_manager_test_api\ApiController->createAndApplyStage(Object) (Line: 73)
├ Drupal\package_manager_test_api\ApiController->run(Object)
├ call_user_func_array(Array, Array) (Line: 123)
├ Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
├ Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
├ Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
├ Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
├ Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
├ Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 53)
├ Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
├ Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
├ Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
├ Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 116)
├ Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 90)
├ Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
├ Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
├ Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 53)
├ Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
├ Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 715)
├ Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
├ require('/tmp/build_workspace_29b4565cc0c9b8a68abd3d8a6d0e7b8fGPvmzn/project/web/index.php') (Line: 71)
That's the same experience I had. Most likely it is timing out after 1 sec but still a 200. If you assert that the response is empty it will probably fail and show the timeout error.
Side note I wonder if we can get htmlOutput to work here.
AFK now but there is a constant in one of the test base classes that's setting it to 20 seconds. I'm not sure if there is something else about my local that's causing it to time out though.
FWIW I can replicate the exact error by adding this to the start of \Drupal\package_manager_test_api\ApiController::run
http_response_code(500);
die();
Running this test locally always fails for me, but it fails asserting the expected versions. That means makePackageManagerTestApiRequest is passing.
I found it odd that the Error response: in the output is empty, thinking maybe an error was thrown and system.logging error_logging was set to hide instead of verbose, but that's not the case. I explicitly put some errors in \Drupal\package_manager_test_api\ApiController::run but I could always see the error response in the output.
I then thought maybe there is an OOM or timeout. I didn't have much luck down the OOM route, but I did find that if I add this line to makePackageManagerTestApiRequest it would fail, even after asserting the 200 status code.
$this->assertSame('Finish', $session->getPage()->getContent());
1) Drupal\Tests\package_manager\Build\PackageUpdateTest::testPackageUpdate
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'Finish'
+'<br />
+<b>Fatal error</b>: Maximum execution time of 20 seconds exceeded in <b>/tmp/build_workspace_4d8944e5916659803141418ed3171c99nCcLIM/project/vendor/php-tuf/composer-stager/src/Internal/Finder/Service/FileFinder.php</b> on line <b>68</b><br />'
I'm not sure exactly how that helps, but hoping it can spark some ideas.
Rebased and updated the MR with a suggested fix, updated proposed resolution.
mstrelan → made their first commit to this issue’s fork.
I repeated the original job that failed 1 in 3 times and now it's not failing at all. https://git.drupalcode.org/issue/drupal-3317520/-/jobs/4820216
Not sure what to make of that, maybe we don't need the trait after all. But I think the waitForElementVisible makes sense.
mstrelan → changed the visibility of the branch 3317520-settings-tray-repeat to hidden.
Using WaitTerminateTestTrait seems to allow us to unskip ::testEditModeEnableDisable without splitting to a separate class - https://git.drupalcode.org/issue/drupal-3317520/-/jobs/4819919
But it failed once where I removed the usleep. Instead of usleep, why don't we just use waitForElementVisible? This seems to pass 100/100 times - https://git.drupalcode.org/issue/drupal-3317520/-/jobs/4819970. And once more for good measure https://git.drupalcode.org/issue/drupal-3317520/-/jobs/4819989.
Have created another MR with just these fixes and updated the issue summary.
Perfect. I've reviewed the MR and can see the duplicate has been removed and there is still one Spell-checking job in tact. I checked the pipeline and can see the Spell-checking job is running as expected.