Test for regressions against Drupal core #207
Conversation
|
So now we know, as expected/wanted, the current HEAD of We also want to know:
Let's revert "Fix regression in 8.3.19 for variadic function parameters (#3365993)" to test both, |
…unction parameters (#3365993)" This reverts commit e4e2e22.
|
Good enough for me, let's revert the revert to have only the addition of the new GitHub CI step in this PR, |
…riadic function parameters (#3365993)"" This reverts commit 8be3d86.
|
NIce work. One question about the failed test |
|
Nice catch. Should be fixed now. Fail test showing the fail: Including failed mail: |
|
Excellent. |
|
This will only detect regressions in the sniffs that core has enabled, so there could be things that still get missed. It's good to see that in recent months there has been a push to get more sniffs passing and enabled in core. Here's the current list of things being worked on https://www.drupal.org/project/drupal/issues/2571965 |
|
For example, the regression in constantName.constantStart would not have been discovered because Core has not enabled this rule yet. |
AFAIK, that's exactly why new stuff goes into new rules and it should be a core queue issue to fix new failures on the new rule before enabling it (in the same issue) in The regression test that's proposed in this issue is just that, testing regressions in existing rules. |
|
Yes I'm totally with you on all that. I didn't mean to sound like I was taking anything away from your work - this is a superb addition to Coder's reliability and integrity. |
|
Oh, I wasn't offended/whatever in any way/shape/form, nor do I (strongly) oppose the opinion coder should test for failures on new rules. |
| - name: "Checkout Drupal core HEAD" | ||
| run: | | ||
| cd ${{ runner.temp }} | ||
| git clone https://git.drupalcode.org/project/drupal.git |
There was a problem hiding this comment.
we should use the github git mirror for drupal core for better speed. We should also use the checkout action.
Or is that not good because we don't have a good default git branch on github?
| cd drupal | ||
| composer config repositories.0 composer https://packages.drupal.org/8 | ||
| composer config repositories.1 path $GITHUB_WORKSPACE | ||
| - name: "Install Drupal core dependencies" |
There was a problem hiding this comment.
we don't need the drupal core dependencies at all, we just want to check the code of drupal core.
I'm thinking if we can integrate this whole job into the PHP 8.2 testing run further above? Like we do with PHPStan.
We clone Drupal core and run the local phpcs with drupal core's phpcs config. That should be all that is needed?
There was a problem hiding this comment.
I'm thinking if we can integrate this whole job into the PHP 8.2 testing run further above? Like we do with PHPStan.
Wouldn't that clash with the the important green icon?
Wouldn't we be able to distinguish a fail in coder's code and a regression on core?
Anyway, this issue is getting way to complicated for lil ol' me, so I won't be persuing any chances.
There was a problem hiding this comment.
Well, this pull request already does interfere with the important green icon as you demonstrated if I understand it correctly? Which is fine - if there is a regression in Drupal core we should know about it. Then there is a small chance that it is a false positive and we have to modify this Github action to work around it - which we also accept.
If we have different steps in the action we can see where it failed - either at Coders tests or at the Drupal core regression test.
No worries about the complication, anybody else can pick this up and we continue at a later point. Thanks for the contribution, nice demo in any case!
There was a problem hiding this comment.
??? what happened? This was going so well. I thought we were all in agreement (mostly) and that this was nearing being merged. It was such a good addition to the Coder's stability.
There was a problem hiding this comment.
Not much happenned, I assume?
There's a demo, things can be changed in the code, just not by me, important green icons is something beyond my comprehension/paygrade :)
|
Continuing this work now in #211 |
See https://www.drupal.org/project/coder/issues/3374864