Core automated tests on drupal.org currently have one un-ignored coding standard fault. This was introduced in this commit at 9.1 and has since been propagated into 9.2. I reported it on #3008028-112: Migrate D7 i18n menu links but there has been no action there, hence creating a separate issue to fix it.
See the daily core test results https://www.drupal.org/pift-ci-job/1858608
Simple fix of the file core/modules/content_translation/migrations/d7_menu_links_localized.yml
Patch attached for review
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3178338-2.duplicate-newline.patch | 473 bytes | jonathan1055 |
Comments
Comment #2
jonathan1055 commentedCurrent test output shows
Patch attached to remove the duplicate new line.
Comment #3
jonathan1055 commentedNeeds review
Comment #4
longwaveThanks for spotting this.
Comment #5
jonathan1055 commentedDo you know if the core test failure is a one-off/random fail? Or is it something we need to start investigating? I could not see any open issue for it.
Comment #6
longwaveIt's a random fail in Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest yet again, it just needs someone with permission to force a retest on the branch.
Comment #7
jonathan1055 commentedIs there an issue for the random fail? Can it really be random? I've worked on some contrib issues that seemed random because they very rarely happen and then appear to fix themselves and run for months before failiing again. But there was always a reason behind the chance failure. Not fixing them was just causing more wasted developer time.
Comment #8
longwave#3037436: [random test failure] Make QuickEditIntegrationTest more robust and fail proof
Comment #10
longwaveAnd now QuickEditIntegrationTest hits us here :(
Comment #11
jonathan1055 commentedThanks for the info and link to that issue.
Comment #12
quietone commented@jonathan1055, thanks for noticing and fixing my mistake.
Comment #13
jonathan1055 commentedHi @quietone, everyone makes mistakes, that's no problem, and your's was very small. Two major core committers did not spot the error either!
What surprises me is that the d.o. test config is not set to fail when coding standards messages are thrown. Core currently adheres only to a subset of the full phpcodesniffer checks and that list is tighly controlled. New sniffs (which fail) are not run, so there would be real benefit in making the tests fail for coding standards infringements. Then they could be fixed immediately and the issue we are dealing with here would never need to exist. This must have been discussed many times before.
Comment #14
alexpottCommitted and pushed 7b4112e63b to 9.2.x and c4b3a0f381 to 9.1.x. Thanks!
Comment #17
longwaveNothing to postpone here, this is fixed now.
Comment #18
jonathan1055 commentedThanks @alexpott and @longwave.
Do you have anything to add regarding my questions in #13 about making tests fail if there are coding standards errors? My underdstanding is that this would only happen when core already adheres to that standard, so it would be correct to alert us of the failure at the time, rather than fix things afterwards, like here.
Comment #19
alexpott@jonathan1055 as we move to merge requests we're going to have to do this since we can no longer rely on the git hooks committers are using. See #3178845: Run same checks as committers do on DrupalCI for more.