Test for regressions against Drupal core

Created on 16 July 2023, 12 months ago
Updated 9 October 2023, 9 months ago

Problem/Motivation

Lately we had some regressions creep into this fine module.

It would be nice/necessary to check changes in this module against both Drupal core and contrib.

This issue is for the regression checking against Drupal core only.

Steps to reproduce

Proposed resolution

https://github.com/pfrenssen/coder/pull/207

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Fixed

Version

8.3

Component

Coder Sniffer

Created by

πŸ‡³πŸ‡±Netherlands Spokje

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @Spokje
  • Status changed to Needs review 12 months ago
  • πŸ‡³πŸ‡±Netherlands Spokje
  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    This is an excellent idea, thanks for doing it. I have linked the two recent issues that had regressions.

    Previous discussion about forward testing is on https://github.com/pfrenssen/coder/pull/202

  • πŸ‡¦πŸ‡ΉAustria klausi πŸ‡¦πŸ‡Ή Vienna

    I think this make sense!

    One problem I see: if we improve a sniff in a way that detects more problems in Drupal core then the Coder tests will fail. This will not happen often, but what do we do in that case? We could then simply comment out this new Github action again so that it does not run anymore? Or overwrite core's phpcs.xml.dist to exclude the sniff in question from checking? Should be possible with some hacky search/replace bash commands. Or we could copy and commit core's phpcs.xml.dist and modify it for our testing.

    This new Github action increases the runtime of Coder tests by 4 minutes, so I think that is acceptable as well.

  • πŸ‡³πŸ‡±Netherlands Spokje

    One problem I see: if we improve a sniff in a way that detects more problems in Drupal core then the Coder tests will fail. This will not happen often, but what do we do in that case?

    My personal opinion: Since the improved sniff will only start to work if the version of drupal/coder is upped in the core composer.json, it won't affect core right away.

    If it's decided to use the improved sniff, the issue to up the version of coder should/could be used to fix the new failures.

    I don't see the need for coder to do anything, if all tests pass _except_ the new regression test, it's "just" an indication core should do "something". For me it's not an indication coder has done something wrong.
    Also the "updated deps" daily QA run will already report these new failures, so core can/should already be aware.

    Trying to do anything about the failed regression test is basically always messy IMHO, and, for me, doesn't add anything except for a green icon at the end of a full test run.

    (As always: n=1, other opions are welcomed and may very well make more sense than the above)

  • πŸ‡¦πŸ‡ΉAustria klausi πŸ‡¦πŸ‡Ή Vienna

    But the green icon is super important psychologically and to keep the overview what is passing and what not. If we accept the red failed icon as normal, then other test fails can be hidden within it. I only want to work on projects where tests are passing in the normal state.

    But as I said I think it will happen rarely and then we can always disable this regression test again completely or work around it.

  • πŸ‡¦πŸ‡ΉAustria klausi πŸ‡¦πŸ‡Ή Vienna

    Reworked the Github actions now in https://github.com/pfrenssen/coder/pull/211

    3 Drupal 11.x files are failing with latest Coder correctly, so this is already a good exercise in disabling/ignoring core files temporarily until they are fixed in Drupal core. It was quite simple with the --ignore option when running PHPCS.

    Let's see how tedious this gets in maintaining this exception list in Coder. Every time a new enum file is added to Drupal core we will have to adapt our test config currently.

    Any objections on the approach?

    Thanks Spokje for opening this issue and the inspiration.

  • Status changed to Fixed 9 months ago
  • πŸ‡¦πŸ‡ΉAustria klausi πŸ‡¦πŸ‡Ή Vienna

    And merged, thanks everyone!

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

Production build 0.69.0 2024