Problem/Motivation

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.

Steps to reproduce

See the daily core test results https://www.drupal.org/pift-ci-job/1858608

Proposed resolution

Simple fix of the file core/modules/content_translation/migrations/d7_menu_links_localized.yml

Remaining tasks

Patch attached for review

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

Title: Fix coding standard fail committed to core 9.1 » Fix coding standard fail committed to core 9.1 and 9.2
Related issues: +#3008028: Migrate D7 i18n menu links
FileSize
473 bytes

Current test output shows

1 coding standards message
core/modules/content_translation/migrations/d7_menu_links_localized.yml
line 45	Expected 1 newline at end of file; 2 found

Patch attached to remove the duplicate new line.

jonathan1055’s picture

Status: Active » Needs review

Needs review

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for spotting this.

jonathan1055’s picture

Do 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.

longwave’s picture

It'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.

jonathan1055’s picture

Is 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.

longwave’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3178338-2.duplicate-newline.patch, failed testing. View results

longwave’s picture

Status: Needs work » Reviewed & tested by the community

And now QuickEditIntegrationTest hits us here :(

jonathan1055’s picture

Thanks for the info and link to that issue.

quietone’s picture

@jonathan1055, thanks for noticing and fixing my mistake.

jonathan1055’s picture

Hi @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.

alexpott’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Postponed

Committed and pushed 7b4112e63b to 9.2.x and c4b3a0f381 to 9.1.x. Thanks!

  • alexpott committed 3a243db on 9.2.x
    Issue #3178338 by jonathan1055: Fix coding standard fail committed to...

  • alexpott committed c4b3a0f on 9.1.x
    Issue #3178338 by jonathan1055: Fix coding standard fail committed to...
longwave’s picture

Status: Postponed » Fixed

Nothing to postpone here, this is fixed now.

jonathan1055’s picture

Thanks @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.

alexpott’s picture

@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.

Status: Fixed » Closed (fixed)

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