- Issue created by @dydave
- @dydave opened merge request.
- 🇫🇷France dydave
Quick follow-up on this issue:
Created initial merge request MR!47 above at #2 which should enable complete Tests coverage and code validation on Gitlab CI.
These changes should allow the module to exploit and benefit from all of the current features and configuration options provided by the Drupal templates for Gitlab CI, available at this moment.
The MR currently fails on PHPCS validation job, see parent issue 📌 Fix PHPCS validation job errors Active .
So, once the changes from the other MR are merged, this MR should pass all jobs and Tests correctly.Since all Tests are passing 🟢, moving issue to Needs review:
https://git.drupalcode.org/issue/structure_sync-3517495/-/pipelines/465983The configuration in this MR is really only a suggestion, it is left at the maintainers appreciation to comment/uncomment/remove any sections added in the changes.
Feel free to let us know if you have any questions or concerns on any of the changes in the merge request or this issue in general, we would surely be glad to help.
Thanks in advance! - 🇨🇦Canada mparker17 UTC-4
@dydave, thank you!
- I'm open to enabling
OPT_IN_TEST_MAX_PHP
if you think that it is necessary.- In the past, I've run into more issues with new code not being compatible with old versions of PHP than issues with new versions of PHP not being compatible with existing code. I'm also mindful of the fact that every test costs the Drupal Association (a little bit of) money.
- I'm open to enabling
OPT_IN_TEST_PREVIOUS_MINOR
, andOPT_IN_TEST_NEXT_MINOR
if you think they are necessary.- Generally, I've found that testing the major versions is good enough, because trying to maintain backwards compatibility typically prevents me from being able to use new APIs for a while anyway. I'm also mindful of the fact that every test costs the Drupal Association (a little bit of) money.
That being said, I am respectfully pushing back on a few of the changes in the merge request...
OPT_IN_TEST_NEXT_MAJOR
will always fail, because that currently refers to Drupal 12, and structure_sync doesn't support Drupal 12 yet.- We could change the
core_version_requirement
to start supporting D12, in order to make those tests pass - but until Drupal 12 reaches alpha, D12 tests will start/stop failing randomly as D12 changes. I've found this discourages new contributors who don't understand that...- the failing D12 test is not their fault, and,
- they don't have to fix D12 failures until D12 reaches alpha.
... I've found contributors will sometimes abandon an issue if they don't understand how to fix a test fail.
- My best practice is to only start caring about compatibility with the next major version of Drupal core when it reaches alpha, and to work on compatibility in an issue branch, so it doesn't affect the mainline until all the bugs have been worked out.
- We could change the
- Based on experience with other projects,
_PHPUNIT_CONCURRENT
(or rather, the thing that variable controls, paratest ✨ Run phpunit tests from a single job in parallel Fixed ) often results in random test fails, especially when running Functional and FunctionalJavascript tests, due to the nature of those tests.- As mentioned in the previous point, new contributors sometimes misunderstand random test fails as something that they did incorrectly, which discourages them from contributing (or makes them abandon the issue).
- I generally don't require all jobs to pass, because - as a maintainer - I haven't found that to be necessary:
- Most contributors will already try to make all the tests pass, i.e.: without me asking.
- Any contributors who don't will usually fix it if I ask.
- If I really feel strongly that an issue needs to get merged right away, I will fix the code on my own.
- Requiring all jobs to pass in
.gitlab-ci.yml
will also enforce that on the mainline branches, and on security team branches (i.e.: and prevent me from merging to those branches if those lints fail). If I was to be contacted by the security team → , I have a duty to the Drupal community to get a fix out as soon as possible. If necessary, I want the option to be able to fix security issues right away, and worry about phpcs, eslint, phpstan, cspell, etc. in follow-up issue(s).
In summary, if you can change the merge request so that it only sets
OPT_IN_TEST_MAX_PHP
,OPT_IN_TEST_PREVIOUS_MINOR
, andOPT_IN_TEST_NEXT_MINOR
, I'll merge it for sure.I'm currently hesitant about merging
OPT_IN_TEST_NEXT_MAJOR
,_PHPUNIT_CONCURRENT
, and requiring the linters to pass; but I'm open to new ideas: if you have compelling reasons why I should use those options, I'd be interested to hear them! - I'm open to enabling