#3480767 Causes break in behavior of tests configuration

Hi cmlara,
Yes, when we made the change to use the core file, I saw those 'coverage' lines, and was was wondering whether they are correct for a contrib project. But then we moved on, and I forgot to ask and investigate. It clearly does affect your output, so we need to do something to fix that. We can't have a customised phpunit.xml in /assets, because the file has to vary between core versions, and we can't maintain duplicates for each core version being tested. Ideally we want to use the core file (from whatever core version is being tested) but post-process it to remove customisations that are not applicable to contrib. Or alternatively, re-write the file picking out the specific keys/data items we need, and dropping everything else. I'm sure there is a good solution to this.

Agree that we need to do something about it.

==> not an option as per comment above because the file is different between core versions.

Another option is to identify the lines that we really don't want and change them or update them, but this could be complex.

And I guess another option would be to not use any configuration file (that's how it was before the other issue was merged) and only use core's one via opt-in or something similar. So we'd just add the behaviour that was added in 🐛 browser_output is empty when testing with Drupal 11 Active via opt-in (eg: _PHPUNIT_USE_CORE_CONFIG or similar). I think this one might be the easier to implement and less disruptive.

I've just pushed a first draft of a script, I wrote this before you posted your comments above.

Thanks for the script. I guess this addresses the "coverage" issue. Just wondering if we brought other unexpected things in the other issue. But in any case, "coverage" is what we know about now, so it'd be ok to make the fix for that.

In any case my but this could be complex. was not knowing what the solution might look like at all, so if you feel this is the way to go, please continue and we can discuss further once a solution is in place and has been tested.

This is ready for feedback and discussion. I've left in the debug to display the file before and after.

The D10 phpunit.xml.dist has coverage whereas in D11 phpunit.xml.dist it is source - at least that what it looks like from the comment.

Tested for both here https://git.drupalcode.org/project/scheduler/-/pipelines/335352 - the file is modified as expected, and is used in the phpunit job, we can see Configuration: /builds/project/scheduler/phpunit.xml in D11. Composer 'previous major' is having trouble downloading everything, but that may resolve itself later.

From #3

And I guess another option would be to not use any configuration file (that's how it was before the other issue was merged) and only use core's one via opt-in or something similar.

I chose not to do it this way as I don't think it would be very helpful. We added the config file so that generated .html worked properly, so if anyone wanted the artifacts they would then have to opt in. If they also had code coverage, they would want to opt out. But could not do both simultaneously. That might be a rare case, I know. But I think modifying the file like the script does, and removing the things we don't want, is a reasonable way forward. It may also be useful to have this functionality for other future changes in the phpunit.xml config.

Thanks for the code and also for the reasoning on #9.

I made a question in the MR that might slightly change the logic.

Good point. I pushed a change - tested locally with multiple 'coverage' and 'source' items and this removes them all.
Test MR https://git.drupalcode.org/project/scheduler/-/pipelines/335472

@cmlara - are you OK if I modify your Vault MR19 to test this change?

Believe I have already merged Vault MR19, however feel free to create a new one to test with.

(Haven’t looked at any of the proposed code fix yet)

OK, thanks I will create a new MR

Here's a test on Scheduler when the project has their own phpunit.xml.
https://git.drupalcode.org/project/scheduler/-/pipelines/335494
You can see the ls output in the log and the message "Using the project's existing phpunit.xml(.dist)"

I've created MR21 in your testing issue 📌 [IGNORE] GitLab CI Sandbox Active
https://git.drupalcode.org/project/vault/-/merge_requests/21

It looks like it has worked, because the final job in MR19 had

Code Coverage Report:
  2024-11-09 17:04:38
 Summary:
  Classes:  0.03% (1/3321)
  Methods:  0.24% (44/18454)
  Lines:    0.31% (434/141283)

but the new output in MR21 is

Code Coverage Report:
  2024-11-12 04:45:16
 Summary:
  Classes:  0.00% (0/16)
  Methods: 51.72% (45/87)
  Lines:   79.35% (465/586)

Code looks good to me and the test in #14 shows that we're fixing the issue that we were trying to address.

I left a couple of comments in the MR, mostly to clean up (debug) code that is no longer needed. The rest look good.

I spotted cmlara's comment at the end of the issue summary

add a check for '--no-configuration' flag.

This is important, because if the maintainer has explicitly said they do not want configuration then we should not add the core file. I extended the regex pattern to ! $_PHPUNIT_EXTRA =~ (-c |--configuration|--no-configuration) to check for any of those.

Test with --no-configuration
https://git.drupalcode.org/project/scheduler/-/pipelines/335644

Test with --configuration
https://git.drupalcode.org/project/scheduler/-/pipelines/335699

What do you think about the name of the new script phpunit-xml.php . Is that OK?

Maybe prepare-phpunit-xml.php?

Renamed, removed debug.
Re-testing here https://git.drupalcode.org/project/scheduler/-/pipelines/336191
Ready for review.

Code wise it looks great! and the tests show it working as expected. I'm going to give it a RTBC but it'd be great if @cmlara also gives us the go-ahead.

Thanks!

There's one more line of unnecessary output I will remove, but it will not affect the RTBC

Visually, appears to have restored coverage to previous functionality.

I still question if we should be blindly including all of core's phpunit.xml rather than vetting each change, however only time will tell if that will cause us problems again in the future.

Thanks for confirming the results.

Yes I understand your concern. The problem is that the config file is linked to the phpunit version which has to vary depending on which core version is being tested. So an alternative would be for gitlab_templates to maintain multiple phpunit.xml in /assets and have logic to determine to the correct one to copy depending on which phpunit --version is being used. That would be quite possible, and the benefit is we just edit and maintain those files and do not have to use the new script to adjust the file at runtime. The downside is we have to keep up with necessary changes in those file, as and when Core make them and/or new phpunit versions/configurations are needed.

I can see then benefit in both solutions, and would be OK to explore that solution if necessary ...

I'd say exploring that other only if necessary. For now, letting core provide the right one is the easiest option. If we see that we start fighting it more than benefiting from it, we can go down the route of internal files, but not for now.

I'll merge this shortly.

Thank you. That's the answer I wanted to hear.

I've changed the issue title, as you often use the pre-prepared text for the commit message. Not sure I've got it exactly right, but when looking through the commits it is better to have what is done, not what the problem was.

Merged. Thanks!

Not sure why this didn't come up in the tests, but somebody reported this job https://git.drupalcode.org/project/data_pipelines/-/jobs/3349914 and we can see the _CURL_* variables empty.

I'm doing a fix for that.

I've merged the hotfix.

fjgarlin → credited mortim07 → .

Giving credit as the issue reported in #28 was reported in slack by @mortim07

Oh, that was a bad error. I'm sorry about that. I think what happened is when we test a MR we have to set those two CURL variables manully (so that the test actually yses the new/modified fies). But this masked the fact that they were not created for real. Sorry!

I'm not sure how we could have tested and discovered this error before commiting the MR.

This was the first time we curl a file somewhere other than composer job. Maybe a solution would be to add them to build.env in the composer job, then they would be available in all job, and we'd avoid this problem. I can try that in 📌 Document internal variables and make them consistent using build.env Active

Don't worry, this was a hard one to foresee as we don't often use those variables and the MR testing requires us to set them up.

We can definitely try that, calculate them in the composer jobs, and them make them available to the rest of the jobs. Working on that other issue sounds good.

Pretty good to get the hot fix deployed in under 2 hours of the fault being reported. Thank you.

is when we test a MR we have to set those two CURL variables manually

Do we? Looking at .calculate-gitlab-ref it should pull off the _GITLAB_TEMPLATES_* that we do have to set.

Maybe this was as simple as we didn't need to set CURL urls and that we did was the warning sign something was wrong?

I'm not sure how we could have tested and discovered this error before commiting the MR.

https://git.drupalcode.org/project/quasar_phpcs/-/blob/main/.gitlab-ci.y... is what I'm doing on the component side for job testing.

I had an advantage that I'm able to build from the ground with testing each individual job without using sub-jobs as a key design requirement. I'm also benfiifed by the design being that jobs config should be in the repo outside of the gitlab-ci.yml (to allow local tools to use the exact same config) however however maybe its still possible to even with the design limitations of what gitlab_templates currently has, to shoe-horn something similar in where contrib tests run in the gitlab_templates workspace instead of creating MR's downstream.

Thanks for the above info.

Looking at .calculate-gitlab-ref it should pull off the _GITLAB_TEMPLATES_* that we do have to set.

The testing that I do involves defining the gitlab MR repo and ref directly in the file:

include:
  # MR 289 build.env and internal variables
  - project: issue/gitlab_templates-3487169
    ref: 3487169-build-env-internal-variables
    file:
      - '/includes/include.drupalci.main.yml'
      - '/includes/include.drupalci.variables.yml'
      - '/includes/include.drupalci.workflows.yml'

as per the second section of the documentation. So the _GITLAB_TEMPLATES_ variables are not set, and are not actually used to drive the testing. I do it this way because I have multiple gitlab templates MRs being tested simultaneously, so I have a project MR for each one. Using the form would be tedious as it would require continual changing if I wanted to switch back to testing another MR.

We did do quite a bit of thinking and testing when I first started working on gitlab templates MRs, and we had to introduce the _CURL_ variables for a very good reason. I will dig back and find the issue where we had to do that.

Maybe another part of our testing process, just before merging, could be to run a pipeline from the UI form, as that would have highlighted this error.

It was added here 🐛 _GITLAB_TEMPLATES_REF value in variables needs updating, and provide override variables for Curl Fixed

Ah that’s right _GITLAB_TEMPLATES_* is set globally in Gitlab isn’t it…
Did we ever test the _TEMPLATES_BRANCH = $_GITLAB_TEMPLATES… method ? (Same as we do for DRUPAL_CORE).

Probally can not be done in the 1.x branch as it would require everyone update their templates to include the variable linking, however might but worth adding to our 2.x plans to streamline the design. Would still allow declaring in the file to fit your existing workflow.

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