Test using MR381 with no changes, so installs node_modules as usual. Here is the full pipeline - all passed.
The composer log shows
Top 10 folders by size:
489 web/core
364 web/core/node_modules
118 web/core/node_modules/@ckeditor
66 web/core/modules
39 web/core/node_modules/ckeditor5
37 web/core/node_modules/ckeditor5/dist
30 web/core/node_modules/ckeditor5/dist/browser
18 web/core/node_modules/selenium-webdriver
17 web/modules/contrib
17 web/modules
The downloaded artifact zip is 142Mb. Unzipped 643MB
Test using MR381 with _COMPOSER_YARN_INSTALL: 0
Full pipeline all passed.
The composer log does not show web/core/node_modules. We see
Top 10 folders by size:
125 web/core
66 web/core/modules
17 web/modules/contrib
17 web/modules
16 web/core/assets
15 web/core/lib/Drupal
15 web/core/lib
15 web/core/assets/vendor
13 web/core/tests
13 web/core/assets/vendor/ckeditor5The downloaded artifact zip is 57Mb. Unzipped 261MB, a significant decrease.
The eslint, stylelint and cspell jobs have this, which I don't understand, but is probably nothing to worry about:
➤ YN0007: │ @nightwatch/nightwatch-inspector@npm:1.0.1 must be built because it never has been before or the last one failed
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT > @nightwatch/nightwatch-inspector@1.0.1 build
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT > node preprocessExtension.js
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT 📂 Moving to src folder ...
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT 🚀 Creating .crx file ...
➤ YN0000: │ @nightwatch/nightwatch-inspector@npm:1.0.1 STDOUT ✅ .crx file created successfully!
Note that this only tests ESlint, Stylelint and CSpell, because Scheduler does not have any Nightwatch tests. I've made a couple of comments in the MR but it all looks good.
Yes RTBC I just left a comment in the MR on the description. Do we want to add "The range can be 1 - 32, with 8 as a good default" or something like that.
I made one more edit/suggestion in the doc page.
Do you think the GTD tests are enough? Shall I try this with Scheduler?
I have fixed the variable's description. If this is a blocker for the next release (i.e. updating default-ref) then we can go with this as-is. If we do follow up with the idea of our own variable then the standard one can be moved into the hidden-variables.yml file then.
Back to RTBC if you want to merge and do the release.
In reply to #3
@fjgarlin I will add you back as maintainer of GTD. But strangely I just tried and cannot trigger the downstream pipelines either. Just get the red "An error occurred while making the request."
In reply to #5 and #6
If we were able to do things like skip eslint & stylelint when JS & CSS aren’t changed in a MR, this would be a great approach. That should be a net reduction in compute.
We have 📌 Only run linting jobs if the files changed make sense for the job Active which sounds like what you want.
I was thinking we could create our own variable say _PHPUNIT_DEPRECATIONS which we can allow to have multiple values, to cater for future scenarios, eg. default 0 which would set PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION=0, 1 would set PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION=1 but it could also control other deprecations. If we didn't do that now, are we making it harder to implement in future?
Maybe it is OK to create this real variable anyway, but should it be in .variables.yml or hidden-variables.yml ?
Regardless, this is NW because the variable description needs to be tidied.
Thanks, yes csslint is not run in GitLab CI, we have Stylelint instead. Thanks for the tidy-up.
Added link to summary
This is great news. I'll work on a solution to have that folder as a variable, which can hopefully be changed in the pipeline 'pages' script, so that 'pages:deploy' can use it.
Hey, that's really satisfying that it found two broken links. But of course! that job is only run on commit to main. If I had thought more about this I would have made a temporary change to run 'pages' in the mr to find these two errors before commit.
quietone → credited jonathan1055 → .
Closed the MR now that Core 11.2 has this change merged in #3497431-93: Deprecate TestDiscovery test file scanning, use PHPUnit API instead → . Thanks for opening this issue, it was a good thing to test.
In Gitlab Templates we have a follow-ups 🐛 The /composer directory is not symlinked Active and 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active
Done, was going to RTBC but better for you to check the updated help doc paragraph
Sorry, got delayed with other things. Will do it here.
The command line in run-local-scripts.sh now matches the command in our .gitlab-ci.yml which is a "good thing"
But I noticed that the 'check code' job is already green, so those prettier failures must have been fixed upstream or something.
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/377...
Still it is good that we have this .eslintignore file anyway.
Both jobs are green now.
Which implies that the eslint failures that will be avoided in 🐛 ESLint needs to ignore /vendor folder Active have already been fixed externally.
jonathan1055 → created an issue.
In run-local-checks.sh we have catered for this by adding --ignore-pattern=vendor --ignore-pattern=node_modules so could do the same in the gitlab job. But I think it may be tidier to use a .eslintignore file, which can be active for both local and the pipeline job.
jonathan1055 → created an issue.
There is also the --site-dir public parameter in mkdocs. If this could be changed for MR runs, would that be possible? I think the 'pages:deloy' may be expecting 'public' because I did a quick test with a different directory. But is that something we can explore?
Yes that's good. In that case I may expand that final line in the docs, to say tto check the log when making navigation changes, as different errors are writen at the start and end of the log output.
I've applied your suggestion, it was an improvement, thanks.
When this is committed, are we still going to leave the issue open to investigate markdown-link-check as originally suggested? Internal link errors are shown in the log so they can be fixed, it's just that they do not cause an failure exit code. I'm ok with marking this fixed if you are?
I've removed the temporary lines, and responded in the MR why I think the other line should stay as-is.
This is ready for review. Are you OK with help doc paragraph?
Job https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/56...
Wrote to http://project.pages.drupalcode.org/gitlab_templates_downstream which is the real site, as expected.
Now ticking the 'unique domain' and run as maintainer
Job https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/56...
Wrote http://gitlab-templates-downstream-1d35fd.pages.drupalcode.org
Can you push a change next?
Thanks for doing this.
I guess the main project had "use unique domain" setting on
No it doesn't, see attached. I think this confirms what we thought on the parent issue. When a non-maintainer runs the job, the url has both the issue number -3531742 and the extra digits -98ccf5, even if the 'unique domain' is not ticked.
https://gitlab-templates-downstream-3531742-98ccf5.pages.drupalcode.org/
I will push a change to this MR too, and I expect it will generate the real doc site, without the issue number or digits. Then we can repeat with the 'unique domain' ticked and get both results from that scenario.
OK that's sounds like a good idea, as you already know what we are trying to do. I have just done it.
Small thing, but worth fixing. When using --debug you don't expect the output files to be corrupt. Especially as that is probably precisely the time when you need to examine the files.
I have also just checked that this works when there is no '--debug' in the original string, and it does work OK
I have the mechanics of this working now in Scheduler MR194. The patch has to be allowed to fail because the two .gitlab files are not present, but that does not matter, the other two files are patched OK. I have added a custom phpunit config, called custom.xml just to make sure we can tell it is distinct. I have defined a testsuite in it, but using --testsuite gives ERROR: Unknown argument '--testsuite' as that is not a recognised parameter in run-tests.sh
https://git.drupalcode.org/project/scheduler/-/jobs/5637581
I then tried just using the name of the testsuite, which is 'only-token-test' and that gives ERROR: Test group not found: only-token-test because of course the default type of filter is on test group, not test suite.
https://git.drupalcode.org/project/scheduler/-/jobs/5637673
Do you have other suggestions on how I can verify that this config is active? If I pass in --module, --class or --file then those arguments will take precedence.
@mondrake and @smustgrave, Would you like me to use this patch in a Contrib pipeline to try it and see if I can use an alternative phpunit.xml when runing phpunif with run-tests.sh?
Added screenshots to show the problem, because these are not viewable when the last run is clean.
Testing with GTD MR8
As-is with --debug in all phpstan commands, we get invalid .xml and the red errors in the report tab
https://git.drupalcode.org/project/gitlab_templates_downstream/-/merge_r...
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
Now using this MR375
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
The MR report tab is green with no .xml errors, even though we still get the debug output in the viewable log.
Ready for review
New logic for 1/0 works
With _MKDOCS_STRICT: 1 (the default)
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/56... - fails red
With _MKDOCS_STRICT: 0
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/56... - passes green
I have also added a section in the pages.md - which can be viewed here
Ready for review.
Tested with allow_failure: true in GTD MR24 and the job ends amber not red as we hoped.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/56...
If the single 'warning' is not present (temporarily commented out the line which creates that) then the job does indeed end green. So all those internal checks which are shown as 'info' in the log do not cause a exit code.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/56...
It's a shame that the internal bad links and anchors do not throw a detecable exit code. But it is still worth adding --strict. Maybe we could still investigate https://github.com/tcort/markdown-link-check as that would also verify external urls.
NW for the change to variable as per #11
That's a great idea! I have opened 📌 Avoid using PHPStan --debug when generating reports Active
Merged MR22. Thanks.
jonathan1055 → created an issue.
I've added variable _MKDOCS_STRICT which defaults to --strict but can be erased if required.
The other way would be to use a 0/1 flag and derive the value. We've used both approaches previously, but mainly when there are multiple options. Now that I have typed up this comment, maybe this one should actually be a 0/1 switch, as I think that's what we have used more times than not.
Here is what I have seen so far, maybe we can work out from this:
Coding Standards
📌
Formatting fixes
Active
1. MR run by maintainer writes to the 'real' site
https://project.pages.drupalcode.org/coding_standards/
2. Same MR pipeline by non-maintainer (me)
https://coding-standards-3527545-5994d4.pages.drupalcode.org
This has the issue fork and the extra digits
Gitlab Templates Downstream
📌
Add documentation .md files to test the pages job
Active
3. MR run by maintainer with the 'use unique domain' box ticked
https://gitlab-templates-downstream-1d35fd.pages.drupalcode.org/
This looks like 2 but without the issue number
4. Then by maintainer after unticking the box
https://project.pages.drupalcode.org/gitlab_templates_downstream/
This is the same as 1.
This does not cover all the scenarios. We could invite a non-maintainer to run MR pipeline in GTD, and I assume the result would be like 2 if the 'unique domain' is not ticked. Then find out what happens when it is ticked.
Here is the mkdoc API reference https://www.mkdocs.org/dev-guide/api/ but I don't know where to start with that.
Yes, this is a good discovery. Thanks for posting that help output.
I'd set the strict option by default for all pages jobs
I was wondering if we need to have it controllable by a pipeline variable, it would be on strict by default, but we don't know if there are edge cases that cannot be fixed and we are forcing the job to fail. So having a variable to override and not add the --strict would cater for that scenario.
Thanks. MR22 merged.
I have tested this using GTD MR22 with
BEFORE_SCRIPT_ACTIONS: pages-fail and it does fail, which is great.
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515276/-/j...
INFO - The following pages exist in the docs directory, but are not included in the "nav" configuration: - page-not-reachable.md
WARNING - A reference to 'not-a-page.md' is included in the 'nav' configuration, which is not found in the documentation files.
INFO - Doc file 'link-failure-page.md' contains an absolute link '/absolute', it was left as is.
INFO - Doc file 'link-failure-page.md' contains an unrecognized relative link 'non-existent-link', it was left as is.
INFO - Doc file 'link-failure-page.md' contains a link '#no-anchor-in-same-page', but there is no such anchor on this page.
INFO - Doc file 'link-failure-page.md' contains a link 'index.md#no-anchor-in-other-page', but the doc 'index.md' does not contain an anchor '#no-anchor-in-other-page'.
Aborted with 1 warnings in strict mode!
NR for feedback, but there are still (atleast) two things to check:
- We get a mix of 'info' and 'warning' messages. From the final line it looks like only the 'warning' gives the error exit code. But we need to check this.
- Test with
allow_failure: trueto see if that allows the job to end amber not red
MR23 is ready for review. It adds a BEFORE_SCRIPT_ACTIONS value of "pages-fail". The logs show the expected warnings, but I have not set the --strict option yet. That will be done when I use this MR in the changes for MR374 on
✨
Use markdown-link-check to verify .md links
Active
Ready for review.
Ah, that's good. Yes I will test the -s option here.
If it is the --debug flag then that sounds like a bug upstream? I re-ran without it and the results are different, but not totally clean. The junit.xml file no longer has the file names at the top - https://project.pages.drupalcode.org/-/gitlab_templates_downstream/-/job...
The test summary page is now green, but it still shows xml error
Base report parsing error: JUnit XML parsing failed: 1:1: FATAL: Start tag expected, '<' not found
Previously we had errors for 'Base report' and 'Head report' and I don't know if that is significant. Maybe that error has always been there.
So in summary, adding --debug does not fail the job, but produces invalid junit.xml for viewing. That's poor! I think for these test jobs maybe we will just add the variables: _PHPSTAN_EXTRA: --debug in the job definition when working on PHPstan, and not have it committed permanently.
Sorry I did not see that open issue, I would have posted there. I have now added a comment on that issue.
In #3515276-7: Add documentation .md files to test the pages job → #7-9 you showed me the setting for switching between the real url and a generated one with digits in. When I was testing the doc pages build on 📌 Formatting fixes Active my MR runs were built to that url, which was perfect as it did not alter the live site. Is there a way we change that setting within a pipeline? Or can we document the feature, to allow a maintainer to swap the setting when they are working on changes to the .md files? The added digits in the url are constant within a project, so that url is 'fixed' for all testing, not random each time.
Thanks avpaderno, I have triggered the 'd9-basic' downstream branch.
We also have a mechanism to force failures in some of the GTD testing branches. You may like to try adding the variable
variables:
BEFORE_SCRIPT_ACTIONS: 'cspell-use-madeupword' into the '→ GTD D9 Basic': downstream job in .gitlab-ci.yml, to see that it fails.
Just checked, and confirm that all of the redundant links have been removed, and also you have fixed all of the info/warnings about missing urls. So this is RTBC from me.
The "pages" job does actually give warnings if internal links are not found, see this Coding Standards MR job. I case not everyone can view that log, it says at the start:
$ mkdocs build --site-dir public $_MKDOCS_EXTRA
INFO - Cleaning site directory
INFO - Building documentation to directory: /builds/project/coding_standards/public
INFO - Downloading external file: https://unpkg.com/mermaid@11/dist/mermaid.min.js
INFO - Doc file 'css/architecture.md' contains an absolute link '/files/10.progress.png', it was left as is.
INFO - Doc file 'php/coding.md' contains an absolute link '/node/542202', it was left as is.
INFO - Doc file 'php/coding.md' contains an absolute link '/node/542202', it was left as is.
INFO - Doc file 'php/documentation.md' contains an unrecognized relative link 'todo-to-do-notes', it was left as is.
INFO - Doc file 'php/naming-services.md' contains an absolute link '/node/2083979', it was left as is.
INFO - Doc file 'php/psr4.md' contains an absolute link '/project/bootstrap/issues/2763861', it was left as is.
Then at the end we also get
INFO - Doc file 'css/architecture.md' contains a link '#formatting', but there is no such anchor on this page.
INFO - Doc file 'css/review.md' contains a link 'format.md#rtl', but the doc 'css/format.md' does not contain an anchor '#rtl'.
INFO - Doc file 'php/documentation-examples.md' contains a link 'documentation.md#callbacks-for-built-in-PHP-functions', but the doc 'php/documentation.md' does not contain an anchor '#callbacks-for-built-in-PHP-functions'.
INFO - Doc file 'php/documentation-examples.md' contains a link 'documentation.md#hook-menu-page-router-callback-functions', but the doc 'php/documentation.md' does not contain an anchor '#hook-menu-page-router-callback-functions'.
INFO - Doc file 'php/documentation.md' contains a link '#file', but there is no such anchor on this page.
INFO - Doc file 'php/documentation.md' contains a link '#defgroup', but there is no such anchor on this page.
INFO - Doc file 'php/documentation.md' contains a link '#Plugin', but there is no such anchor on this page.
INFO - Doc file 'php/documentation.md' contains a link '#types', but there is no such anchor on this page.
INFO - Doc file 'php/documentation.md' contains a link 'documentation.md#arrays', but there is no such anchor on this page.
INFO - Doc file 'php/documentation.md' contains a link '#ref', but there is no such anchor on this page.These are not highlighted in the log and can be easily missed if you are not looking for them. Is there any way we can make them more prominent, or even make the job end with an amber warning instead of green pass?
I added this back in but the changes here are deployed to the 'real' URL. Did I make a mistake?
That is the expected behavior when those lines are added in a MR and the pipeine is run by a maintainer. Unfortunately we don't have any solution for this yet, and its not satisfactory at all. If I were to submit a pipeline on this MR the pages would be built to a different url, and the live site would not be affected until the MR was committed. This is the same as what I noted in the issue summary on 📌 Formatting fixes Active . I have updated Gitlab Templates issue 📌 Only rebuild documentation pages when source has changed Active so hopefully we can make some progress there.
I also noticed a bunch of information errors
Well spotted. I wonder if those could be highlighted better in the log, or somehow we draw attention to them. Yes, definitely fix in this MR (I think you have done some already)
We also need to investigate an easy way to allow the pages job to be run in a MR and for the target url to be different from the real live documentation site. For info, see the summary notes on 📌 Formatting fixes Active . Maybe we can utilize the alternative url setting for this?
MR results show this problem with each of the phpstan jobs, but they ran OK. I don't really get it.
This is ready for review. It's needed to assist testing MR371 on 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active
Thanks for that. In
#3527579-9: Add #[Group] attributes for Core 11.2 and PHPUnit 11 →
I used your suggestion of setting the plain variable PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION: 0 and this does prevent the internal phpunit deprecation log messages. I am also working on making some changes in our downstream test branches to demonstrate the issue.
To implement this, we could just add the new variable to the hidden-variables file and document how to use it. But I think it may be better to add our own differently-named variable that we set, and the users can customise, or enter in the pipeline form, then we convert that to the actual value for PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION: 0 in the script. This will give us the ability to add further options or enhancements in future, if they become available. I will add that to the MR and see how it looks.
The third commit adds a @dataprovider and we get more deprecations in phpunit 'next minor'
Pipeline https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
Specifcally gtd3 before had Tests: 1, Assertions: 3, PHPUnit Deprecations: 4 see log
but now has Tests: 2, Assertions: 6, PHPUnit Deprecations: 9. see log
The second commit diaplays deprecations in the 'next minor' phpunit job. The jobs end amber due to phpunit deprections (some in this module, others in core files)
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
The first commit splits the phpunit job into three parallel jobs, based on the @group
All pass green
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
jonathan1055 → created an issue.
Yes partly. It removes the internal phpunit deprecations (such as core files not having new #[ ] attributes, but does not remove the phpunit test discovery warrnings (which are only a couple of hundred lines, so no big deal on file storage, but just makes the log long for viewing) and it does not remove any 3rd-party deprecations in test dependency modules (or which there can be lots, if you use three of four external modules for test compatibility, such as commerc, addres, token). That's where the bulk of the 30,000 lines came from. But maybe Scheduler is atypical in that way, and most contrib wont have a massive increase in log size.
I tested the new behavior on #3527579-9: Add #[Group] attributes for Core 11.2 and PHPUnit 11 → and will put this into 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active soon.
and just another warning - the size of the logs can massively increase with all the deprecations and internal messages. Schedulers main group now produces more than 30,000 lines of log every run.
Here's the full pipeline with MR372
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/525998
The 'current' phpunit all end green, but we do get extra log messages about internal phpunit warnings
At 'next minor' - which now shows Drupal version 11.3-dev es we get the usual deprecations we have had at 11.2.x, because that's what I've set for 'next minor'
So all OK to proceed and merge, but I suggest we have a think about when to make this default-ref, i.e. not immediately.
The changes look fine and all GTD branches run OK. But those phpunit jobs are still minimal. I'd like to test this on Scheduler too. I'm not expecting any problems, but better to know, given the major phpunit test discovery problems and changes in 11.2.x.
Thanks, yes that worked. After adding #[DataProvider()] to go with each @dataProvider in the javascript tests we still had the message There were 9 PHPUnit test runner deprecations followed by 8 tests triggered 11 deprecations
https://git.drupalcode.org/project/scheduler/-/jobs/5598340#L663
With PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION: 0 we just get straight to 8 tests triggered 11 deprecations
https://git.drupalcode.org/project/scheduler/-/jobs/5599415#L661
Thanks also for raising 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active
jonathan1055 → created an issue.
Oh yes absolutely, and no apology is needed :-) I just didn't want people to think that all the content had been checked by me. I recall that you checked some 'last updated' dates previously and there was not much (if anything) that had been changed on the existing pages.
I have added your second follow-up issue to the summary.
Are you saying that we just need to set the environment variable PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION to 1 or 0 and the phpunit binary will use that value? I've just looked in run-tests.sh and there is no reference to that variable or the --fail-on-phpunit-deprecation option.
I will try it and see what happens.
For #27.1 I repeat that it was not me who did any content review. There may have been updates to some pages since the initial commits.
There was a discussion of the tasks and sequencing on 📌 Convert Coding Standards to GitLab pages Active so maybe it would be good to check against that, as the issue was closed before the tasks were marked off. It looks like most things have been covered though.
This is a good idea. I saw this issue a few days ago, and thought 'yes nice'. But now that I am hitting the PHPUnit deprecations in my work to add attributes, it has become 'yes must' !
Thanks for the info. But you don't have to remove the deprecated @ metadata just yet, you only need to add the equivalent #[ ] version to prevent the deprecation messages. Both can exist, and the tests run at both PHPUnit11 and PHPUnit10 without a problem. At least, they do with Contrib @group and @dataprovider, because I've done that here.
Now that this has been re-committed to 11.2.x I presume the idea is to fix all the Core tests so that they do not produce PHPUnit test runner deprecations? Contrib tests get deprecation warnings for Core classes and methods that don't have the new attributes yet. For example a javascript test that extends use Drupal\FunctionalJavascriptTests\WebDriverTestBase gets warings that the failOnJavaScriptErrors() method contains doc-comment metadata.
See https://git.drupalcode.org/project/scheduler/-/jobs/5596390#L670
Here is the failOnJavaScriptErrors() method in webDriverTestBase
I've not found any open core issues to start takling this task yet, or maybe I missed it. I can open an issue if this is the plan.
After adding group attributes for scheduler_js
Before: There were 27 PHPUnit test runner deprecations
After: There were 18 PHPUnit test runner deprecations.
The 9 removed messages were all
Metadata found in doc-comment for class Drupal\Tests\scheduler\FunctionalJavascript\SchedulerJavascriptDefaultTimeTest. Metadata in doc-comments is deprecated and will no longer be supported in PHPUnit 12. Update your test code to use attributes instead.
So that's good. Even though the @group scheduler_js<code> is still in the doc-comment, providing there is also the new <code>#[Group('scheduler_js')] then the phpunit test runner deprecation is not displayed. That should mean we can fix the code for the new core 11.2+ and still support testing at the old core versions.
Here is the updated site
https://coding-standards-3527545-5994d4.pages.drupalcode.org/
I've now removed the temporary code again. MR6 ready for review.
Actually the doc site has not been rebuilt because we don't have the temporary code in place to trigger the "pages" job. I want to make this more user-friendly for checking documentation fixes. See 📌 Only rebuild documentation pages when source has changed Active and 📌 Add documentation .md files to test the pages job Active .
MR6 is ready for review.
The site is built at
We need to lend support for ✨ Use markdown-link-check to verify .md links Active then this could have been picked automatically and fixed before merging.
First round of 3rd-party ignores. Big improvements made:
scheduler- 23 tests triggered deprecations (12-15 deps each). Log is 12K linesscheduler_api, each 12 - 13 deprecations, log is 2,900 linesscheduler_js- each test triggered 11 deprecations. Log is 1,500scheduler_kernel- each test triggered 7-9 deprecations. Log 890 lines (no change).scheduler_rules_integration- each test triggered 16 deprecations. Log is 3,100 linesscheduler_workbench- 22 deprecations. Log 880 lines.
Need to remove the 3rd-party deprecation warnings so that we can see what actually needs to be done for Scheduler. Initial results as-is before any work, split by test @group:
scheduler- 23 tests triggered deprecations (all range between 54 and 74 deps) = too many add up accurately. Log is 41K linesscheduler_api, 63 + 58 + 59 + 54 = 234 deprecations, log is 8K linesscheduler_js- each test triggered 54 deprecations. Log is 3K, not too big.scheduler_kernel- each test triggered 7-9 deprecations. Log 890 lines.scheduler_rules_integration- each test triggered between 67-74 deprecations. Log is 8K linesscheduler_workbench- 68 deprecations. Log 1300 lines.
The work to ignore 3rd-party deprecations is too important to be bundled in with this MR241 so it will be done separately in MR244 on 📌 Add #[Group] attributes for Core 11.2 and PHPUnit 11 Active
Then both of the Schduler issues can be worked on once we can actually see the Scheduler problems, and not be swamped by huge logs of 3rd-party module problems.
Actually, on #27 point (1) I did not do any of that checking. I thought you did most of it, and maybe @borisson_ on 📌 Convert Coding Standards to GitLab pages Active but it wasn't me. I helped with the links, the gitlab-ci file and jobs, and the spelling and linting, but not any of the actual content of the pages.
Hi Mondrake, thanks for raising this, following up the testing on
📌
Test with 11.2.0-rc2
Active
. The Gitlab Templates phpunit jobs are run in the top-level $CI_PROJECT_DIR. So to avoid any confusion, I want to check that you are saying we have a problem in scripts/symlink_project.php. This is where we create symlinks in, for example, /builds/project/scheduler/web/modules/custom/scheduler pointing back to the project's own files files in /builds/project/scheduler
So your problem/request is that we need make an additional symlink somewhere. Is that we create, for examlpe /builds/project/scheduler/web/composer point to ? where is the source folder?
Yes you are right that the yml standard for indentation is 2, and this is also enforced in the core .eslintrc.jon setting. For simple yml structures it is easy to see that two spaces are always used, but mkdocs.yml has a more complex nested structure. Here is an example which I did not change, it was alerady correct:
plugins:
- search
- privacy:
enabled: !ENV [CI, false]
The "enabled" is indented 2 spaces from the start of "privacy" which actually menas 4 spaces from the '-' before the 'privacy'. But this looks fine and we did not complain. It looks at a passing glance that indentation of 2 is adhered to.
In a menu of nested links, the same principle applies:
nav:
- Drupal Coding Standards:
- Coding Standards: index.md
But in this case it looked wrong to you, because the new line also starts with '-'. The '- Coding' is indented 2 spaces from the start of the word 'Drupal' so it is actually 4 spaces from the '-' in front of Drupal. This may appear odd but it is what the eslint job is programmed to expect and report on. So I suggest we leave it as-is in the MR and we keep the eslint job running. The alternative is not to run eslint at all, but that would likely be worse as we'd potentially miss other violations of standards.
Ah thank you for explaining that. I see now that your comment in #90 "but the actual list of tests to be executed will be missing those tests that errored during discovery." is actually the "list of tests discovered will be missing ..." but the full list for the intended module should still be run (providing the errors are only in 3rd-party modules). That sounds perfect.
I had just written the below, simultaneously, but copy/pasting here anyway.
the issue reported in #76 is due to gaps present elsewhere, that the PHPUnit discovery process legitimately cannot deal with.
It seems that PHPUnit 11 is obviously a lot more picky about test discovery than PHPUnit 10, which easily found all the @group mygroup and did not complain at all.
But yes I agree, get this in 11.2.x then those tests at Next Minor will start running again. If the output clearly lists the skipped tests and the reasons then at least we have something to work on. But if they are skipped due to 3rd-party module problems then that will me we lose test coverage until they can be fixed. In the earlier 11.2.x work, those were merely reported as deprecations in 3rd-party modules wich could be ignored and the tests ran. Anyway, we have to move forward, so yes RTBC+1. Good work mondrake, thank you for taking this on.
New core issue
📌
Allow indicating alternative phpunit.xml than core's when testing via run-tests.sh
Active
will make it easier for Contrib to name a custom phpunit.xml.
But that work does not replace this issue, they work together.
I just wanted to check that it works with plain phpunit binary (_phpunit_concurrent=0) and it does
https://git.drupalcode.org/project/scheduler/-/jobs/5563832
So now restoring back to using run-tests.sh (_phpunit_concurrent=1)
New error must be unrelated
Error when performing the request to https://repo.yarnpkg.com/4.9.1/packages/yarnpkg-cli/bin/yarn.js; for troubleshooting help, see https://github.com/nodejs/corepack#troubleshooting at fetch (/usr/lib/node_modules/corepack/dist/lib/corepack.cjs:22051:11)
Well we may argue there are too many ways to filter and get the tests needed for running, but we need to support them all ATM
Yes, the phpunit binary supports --group mygroup so it is important that we have the same (similar) functionality in run-tests.sh. You can see in the Scheduler .gitlab-ci.yml I have added a bit in before_script to allow swapping between _PHPUNIT_CONCURRENT: 1 and _PHPUNIT_CONCURRENT: 0 without any further changes. This has proved very useful in my Gitlab Tempates development work.
I could replicate the issue in Image Effects ... by choosing to select tests by group instead of by directory.
Exactly. Yes that is one of the documented ways we show how Contrib can use their Gitlab variabls to make customization. So other projects useing that method would also break with this change.
This is a very good investigation and it is great that you have isolated the problem. Regarding core issue 📌 Allow indicating alternative phpunit.xml than core's when testing via run-tests.sh Active we also have 🐛 beStrictAboutOutputDuringTests=true or failOnRisky=true causing test failures due to PHP8.4 Implicit Nullable deprecation Active in Gitlab Templates, which may help with this.
I don't know how common it is for Contrib to use the @group way to select tests, but I think I'm right in that it is a documented way that run-tests.sh can be used. I have started the process of adding the new attribute meta,
📌
Add #[Group] attributes for Core 11.2 and PHPUnit 11
Active
but that is taking more effort, because the deprecations are still being shown (at least they did when run against 11.2 before the lastest commits on
📌
Deprecate TestDiscovery test file scanning, use PHPUnit API instead
Active
)
Thanks for looking at this. I've pushed a change to skip all the other variants and jobs we are not interested in, and also only run two of the parallel streams automatically. The others are now manual. Makes the whole thing quicker.
jonathan1055 → made their first commit to this issue’s fork.
The actual command that runs the test in these jobs is
sudo -u www-data -H -E php web/core/scripts/run-tests.sh --php '/usr/local/bin/php' --color --keep-results --concurrency 32 --repeat '1' --sqlite 'sites/default/files/.sqlite' --dburl mysql://drupaltestbot:drupaltestbotpw@database/mysql --url http://localhost/web --xml /builds/project/scheduler/web/sites/default/files/simpletest --verbose --non-html scheduler_js
It is the final argument, 'scheduler_js' in the above example, which in 11.1 runs the tests annotated with @group scheduler_js but which fails at 11.2
Also the phpcs job (that currently runs) can be ignored, because Gitlab Templates has been corrected to better determine if php files can be checked. On the next release of the templates this job will not run.
All jobs are green, including eslint.
I think the mkdocs file was clean before, but the new links had the wrong indentation. That can be fixed easily, then we can run the eslint job. I can do that here if you like, and also remove the temporary code.
I know this was going to be committed to 11.x but was surprised to see it also committed to 11.2.x
As soon as Gitlab Templates makes the shift from 11.1 to 11.2 as 'current core version' this will break some existing contrib pipelines, see https://git.drupalcode.org/project/scheduler/-/jobs/5557961 This phpunit job uses the documented @group to divide the tests into parallel streams. I know we can also add the #[group ] metadata, but until contrib has a chance to do that then some pipelines will break. Maybe that's acceptable, but I just wanted to let you know, and to understand the process for Contrib changes.
jonathan1055 → made their first commit to this issue’s fork.
I added debug to simpletest-db to check what we get for SIMPLETEST_DB and the log shows
_TARGET_DB_TYPE=mysql, SIMPLETEST_DB=mysql://drupaltestbot:drupaltestbotpw@database/drupal_database
I also tried adding echo "show databases; show grants;" | vendor/bin/drush --root=$_WEB_ROOT sql:cli both before the tests run and after, but both give the error
In SqlBase.php line 115:
Unable to load Drupal settings. Check your --root, --uri, etc.
The echo pipe drush is correct (checked locally), so either --root=$_WEB_ROOT does not apply here like it does elsewhere, or the database is not available, or something. I also tried removeing --root and got the same result. Is the database deleted after the tests have completed? I'm not sure of the sequence or architecture of installing in this job.
Re-run and we now have failures in d9-basic Max PHP (which passed before) and d10-plus but only Max PHP (so the failure is Previous Minor did not happen this time). It does appear to be random but failure high chance of failure with these two GTD bramches.
Both D7 pipelnes pass. Have we ever seen a failure of this in D7?
This has essentially been RTBC since comment #13 on 7th April. What can we do to get it merged, so save you having to keep rebasing?
Hi cicciobat,
Could you look at the list of 3rd-party modules in #7 and let me know which of those you also have. I suspect that one of them is intertering, and if we could narrow down the list it would make elimination easier.
jonathan1055 → changed the visibility of the branch 3463044-different-mysql-database to hidden.