- Issue created by @Spokje
- Status changed to Needs review
12 months ago 11:04am 16 July 2023 - π¬π§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.
-
klausi β
authored c3f8e282 on 8.3.x
test(github): Add Drupal core regression testing (#3374864)
-
klausi β
authored c3f8e282 on 8.3.x
- Status changed to Fixed
9 months ago 1:16pm 9 October 2023 Automatically closed - issue fixed for 2 weeks with no activity.