Problem/Motivation

  1. Reduce disk space on local server. Automated tests greatly increase the size of a bare Drupal installation on disk. (As of 609912bced (8.8.x, July 15, 2019), they accounted for 43M of 94M (46%).) For many sites, especially simple ones, they provide no benefit to end users in production. The number and size of files can also be a problem for users of shared hosting (c.f. #2338671: Create a smaller Drupal core download, removing tests, for production sites.)
  2. Test files can contribute to security vulnerabilities. See comments #21/#24/.
  3. Including test files increases download times and the load on drupal.org.
  4. Having a smaller code base may improve Development experience.

It's common to apply patches even on live sites. Patches often contain changes to test code and would by default fail if it is missing (see #33. Therefore as a dependency of this issue, we need an enhancement to the patching mechanism to allow applying patches when test code is missing - see this issue (this was not implemented in 2.x and was closed. Composer-patches^2 supports adding custom patchers (it would be easy to create one for Drupal exclude) and the built-in freeform patcher with a small shell script could implement the closed composer-patches issue. See comment #104.

Obviously some sites specifically want to include the tests, so there must be a mechanism for that as simply as possible. This issue should try not to make this scenario worse or use more resources than in the past. In particular note that if we force sites to 'prefer source' to get the test files then this is much worse for the load on Drupal.org servers, see #22.

We need to carefully consider what counts as test code. One commenter (#37) noted that they would be happy to lose the specific core module tests but they would need the testing framework. We could consider including the framework even when excluding tests, depending on what portion of the code size it accounts for and how many sites we judge are in this category.

The primary target of this issue is Drupal Core. However we should ensure that whatever we do is generic so that any contrib projects with lots of tests can copy the same approach.

Proposed resolution 1

Use the approach documented in Use gitattributes to keep your tests out of other people’s production to exclude all tests/ directories from release packages. Since the Drupal.org packaging system already uses git-archive to create releases, the change would involve nothing more than the addition of a few lines to our .gitattributes file and would take effect immediately. Note: This would have no effect on Git clones, so neither the testbot nor core contributors would be affected.

In this scenario, anyone who wants tests uses prefer-source. We need to test and document exactly how to do this targetting just one composer project. A downside is that downloading source is considerably slower and will adversely affect Drupal.org servers.

1A: If this is estimated to be a major problem then a possible mitigation would be to produce a second composer project something like drupal/core-dev that replaces drupal/core and includes test files. However this becomes a non-trivial change to Drupal.org infrastructure and there might be further side-effects e.g. the extra storage for two sets of releases.

Alternative resolution 2

Remove test files after download using the vendor hardening Composer plugin: #3086277: Add a composer plugin to clean up Drupal test directories. An advantage is that this doesn't force anyone to prefer-source in order to get the tests.

However there are various disadvantages

  • Does not solve goal 3 at all.
  • It requires sites to "opt in" by requiring drupal/core-vendor-hardening, which drupal/recommended-project currently does not do. We could fix that, but it wouldn't help sites that are already installed. It's somewhat counter-intuitive to use a tool named "vendor hardening" to harden core and we would need to fix #3104291: drupal-core-vendor-hardening should not assume vendor directory to allow it.
  • Any site that requires a single test would AFAICS need to disable vendor hardening globally.
  • Vendor hardening currently makes composer status pretty much unusable because every single deleted test file is displayed and any 'real' changes are lost amongst that.

Alternative resolution 3

Make all of the test infrastructure into a subtree split library (drupal/tests?) which can be optional. You'd require-dev it in your composer.json file, or remove it as you wished (#2866082: [Plan] Roadmap for Simpletest).

Contrib modules would not be able to replicate this method.

Alternative resolution 4

See #82 and #89

Change drupal/core to omit all test code, and introduce a new drupal/core-with-tests package that includes drupal/core with all the test code.

Then use composer's replace config in CI to replace drupal/core with drupal/core-with-tests (or whatever it is called)

Contrib modules would not be able to replicate this method.

Remaining tasks

  • Decide which resolution to use.
  • Decide which directories to exclude - maybe keep the test infrastructure?
  • Fix any issues that are dependencies.
  • Carefully document and test the procedure for sites that do want to keep test code. Consider any possible adverse implications of this for load on Drupal.org infrastructure.
  • Consider possible unintended consequences.
  • Review/test (forthcoming) patch.
  • Write a change notice.
  • Decide when to commit/release the change.

User interface changes

If tests can be considered to belong to the user interface in any sense, the change is that they will disappear from tarballs.

Release notes snippet

TBD.

Issue fork drupal-3067979

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

TravisCarden created an issue. See original summary.

traviscarden’s picture

Assigned: traviscarden » Unassigned
Status: Active » Needs review
StatusFileSize
new500 bytes

Here's the patch. And here are the results:

$ git checkout 8.8.x
$ git archive --format=tar --prefix=before/ HEAD | tar x
$ git apply 3067979-1-exclude-tests-from-releases.patch
$ git archive --format=tar --prefix=after/ HEAD | tar x
$ du -sh before/ after/
 94M	before/
 51M	after/
$ for DIR in before after; do LC_NUMERIC=en_US printf " %'.f files    ${DIR}/\n" $(find $DIR | wc -l); done
 19,064 files    before/
 10,120 files    after/

Savings: 43M/94M (46%) and 8,944/19,064 files (47%).

traviscarden’s picture

Issue summary: View changes
danepowell’s picture

Status: Needs review » Reviewed & tested by the community

Hear, hear! An additional consideration is that many of the test fixtures are compressed files, so proportionally they make up even more of the tarball (for downloads) than they do space on disk. This needlessly impacts download times, Composer install times, d.o hosting charges, folks on metered connections, etc...

And in terms of files on disk, this has the potential to make so many things more efficient, including IDE indexing or any other operation that recurses through the directory tree.

I agree that it's worth considering potentially unintended consequences. I've almost never run Drupal's core test suites locally except for development on Drupal core itself. I can't imagine people do that regularly, and if they do, they could always install Drupal from source (trivial to do with Composer). But it would be worth hearing from the community.

effulgentsia’s picture

This will affect whether people who download/compose/clone Drupal core can run tests, so tagging for product, framework, and release management review.

As I understand it, the impact is this:

  1. People who download Drupal from a tar or zip file (e.g., from the big blue buttons on https://www.drupal.org/project/drupal) won't be able to run Drupal's tests.
  2. People who get Drupal via git clone (e.g., from the instructions on https://www.drupal.org/project/drupal/git-instructions) will be able to run Drupal's tests.
  3. People who get a tagged release of Drupal via Composer (e.g., from https://github.com/drupal-composer/drupal-project), without explicitly asking for --prefer-source, won't be able to run Drupal's tests.
  4. People who use Composer and either explicitly set the --prefer-source option, or get a branch tip (not a tagged release) of Drupal, will be able to run Drupal's tests.

Is all that accurate?

traviscarden’s picture

Yes, @effulgentsia, that is all accurate.

drumm’s picture

As far as I know, that should all be accurate. I’d recommend double checking a contrib project and/or an alpha release using this this technique to triple check. As mentioned in the issue summary, unintended consequences are possible, I can’t think of any offhand. This would also apply to -dev releases’ tar/zip archives.

andypost’s picture

+1 to minify tarballs, in related issue probably something could be useful

danepowell’s picture

I’d recommend double checking a contrib project and/or an alpha release using this this technique to triple check

We recently implemented this for the Lightning distribution, you can verify that it works as expected by checking the latest dev release and seeing that it excludes all of Lightning's tests: https://www.drupal.org/project/lightning/releases/8.x-4.x-dev

I've also documented this here: https://www.drupal.org/node/1068944#exclude

catch’s picture

Issue tags: +Needs change record

I think #5 is acceptable but it'll definitely need a change record, could also see arguments for postponing the change until 9.x. Also not-untagging so other framework/release people can chime in.

tstoeckler’s picture

I think this makes sense, I was wondering, though, whether it makes sense to also exclude core/modules/simpletest? Can be a follow-up, though, don't want to hold this up, just putting it out there.

webchick’s picture

The proposal outlined in #5 makes sense to me. I can think of very little times it would come up that you'd want to run tests off a tarball and in the event that it did, a git clone is just a command away...

The more barriers we can reduce to a new user getting started with Drupal, the better! :)

Mixologic’s picture

One thing that this might impact is people who have patches to core built into their deploy/build process, either via drush make or using cweagans/composer-patches. Most patches apply to both the code and tests, so they wouldnt apply to a stripped down version.

It's possible we may need to offer both versions.

mile23’s picture

Re: #11: One of the stretch goals of #2866082: [Plan] Roadmap for Simpletest is to make all of the test infrastructure into a subtree split library which can be optional. You'd require-dev it in your composer.json file, or remove it as you wished.

This would very likely still break people's patches if they're patching that infrastructure.

danepowell’s picture

I agree that this could be a breaking change for folks patching core, but this is mitigated by the fact that it's almost trivial to switch to source packages to fix it (composer --prefer-source).

peterx’s picture

We offer a .zip file that is a 27 MB download and works everywhere. We also offer a .tar.gz file that cannot be used everywhere but is only 17 MB. Removing all directories labelled tests and Tests gives us the chance to offer 13 MB download for those people who are hit with big download costs.

beram’s picture

I don't think the following points are good impacts:

3. People who get a tagged release of Drupal via Composer (e.g., from https://github.com/drupal-composer/drupal-project), without explicitly asking for --prefer-source, won't be able to run Drupal's tests.
4. People who use Composer and either explicitly set the --prefer-source option, or get a branch tip (not a tagged release) of Drupal, will be able to run Drupal's tests.

Forcing people that used composer to add the --prefer-source is adding another Drupal specific way to handle project compare to others PHP projects (like Symfony for instance).

--prefer-source is slow: it has to git clone and it is not cached to my knowledge.
Whereas --prefer-dist is faster: it has one ZIP to download and it has local caching.

Furthermore, not providing the tests per default (like all others PHP projects do?) will not encourage people to add tests to their projects. (Promote best practice is important IMHO)

IMHO it may be OK for people using the tarball directly but not the ones using composer.

It is not an argument but Symfony tried this solution and they reverted it later (https://github.com/symfony/symfony/issues/6605). Some links that could be interesting to read: https://github.com/symfony/symfony/pull/32674, https://github.com/symfony/symfony/issues/17749 and all the others linked issues.
I think the comment https://github.com/symfony/symfony/issues/17749#issuecomment-192172162 has some interesting points like :

You can grow the list of pro and contra arguments, the truth is that everyone is right: this is a matter of personal preference.

danepowell’s picture

There's a meta argument that makes this more than just a matter of preference though. If you prefer to have test sources after Drupal removes them, you have a perfectly usable workaround (--prefer-source). If you prefer not to and Drupal doesn't remove them, you're SOL. You just have to eat the wasted bandwidth and time spent downloading a tarball that's twice as big as necessary. Collectively, we're probably talking about petabytes of wasted bandwidth and days of wasted developer time here.

Furthermore, I suspect the vast, vast majority of users don't want or need the built-in tests, and those that do are a more vocal minority. That's not to say that their opinion doesn't matter, I'm just saying we should think about the numbers on this. This is probably especially true for first-time downloaders, who are also going to be more sensitive to how much time/bandwidth it takes to just get started with Drupal.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Seems like this is still actively in discussion.

traviscarden’s picture

Re: @beram

--prefer-source is slow: it has to git clone and it is not cached to my knowledge.
Whereas --prefer-dist is faster: it has one ZIP to download and it has local caching.

True, --prefer-source is slow, but Composer does in fact cache it, so it's only slow the first time. What's more, --prefer-source isn't the only or probably even best tool for the job in this case. Composer's preferred-install config allows you to target individual packages.

But even more importantly, doesn't it seem reasonable to assume that the majority of people who would want to run core's automated tests want to do so precisely because they're contributing to core or at least testing patches to it? In the first case, if they're contributing to core then they surely want to be able to create patches, which means they need an actual Git repository, so they need to prefer source anyway. In the second case, if they're applying patches to core then they're either using cweagans/composer-patches, which does a Git clone anyway (IIRC), or they're most likely already preferring source so they can apply the patches manually with Git. In other words, the users whom it has been argued would be adversely affected by this change would most likely not be affected at all (or if affected, would have a simple workaround) and the vast majority of users would only be benefitted.

Furthermore, not providing the tests per default (like all others PHP projects do?) will not encourage people to add tests to their projects. (Promote best practice is important IMHO)

It is certainly overstating the case to say that all other PHP projects provide their tests by default. The approach in question for excluding them is a documented pattern that's been around at least four years. And given the risks of deploying test code to production, I would argue that excluding them is the best practice we want to promote. At Acquia we're currently considering adopting it as a standard for all of our open source packages.

I doubt this can be reduced to a matter of preference. We've cited concrete risks of not doing it and benefits of doing it. If there are drawbacks, it's still as objective a comparison as any other design decision we make in core. I appreciate the points raised against the proposal, but they seem to me minor and conditional, and I don't find them persuasive. I still stand in favor of the change.

greggles’s picture

With my Security Team hat on I will say that the security angle here is quite important. There are definitely cases, both theoretical and concrete, where having test files included has led to varying levels of site compromise.

More generally: I agree with the logic in #20 that the people most likely to want the test files are also the people 1) most likely to get them already or 2) most able to figure out how to get them. Perhaps there are a few documentation pages that could be updated if this change goes in. Having a list of those in the issue summary would likely make it easier to accept this change. The list of "remaining tasks" also looks like it needs an update.

Mixologic’s picture

If composer patches does in fact already gather the git clones, then thats a good start to alleviating my concerns. What Im reluctant to do is force any user who needs to have a patch (like, every distribution customer out there) to have to use --prefer-source for their build needs.

D.o. is optimized for serving static files like the tarballs as dists, additionally, that is made even better for the end users with our CDN with its geographic distribution. If we have to switch a lot of users over to git clones, thats a *major* additional tax on our infrastructure, *particularly* on update days where there are security releases.

So if people can patch without forcing composer to download all of their dependencies via git clones, then Im much less concerned.

But 'the people who need test files' is a far, far greater number than 'people who plan on running tests', and is the relevant metric here.

fancyweb’s picture

There are definitely cases, both theoretical and concrete, where having test files included has led to varying levels of site compromise.

Could you share some cases with us please? I'm interesting in knowing more about this subject.

Personally, I think the tests should stay when you use composer to get Drupal. Removing them for production is a deploy task.

However, removing them from downloadable releases seems like a good decision. What about just using something else than git-archive to create the releases to be able to remove the tests folder?

greggles’s picture

Could you share some cases with us please? I'm interesting in knowing more about this subject.

Yes, CVE-2017-9841 is a situation of test files having a remote code execution vulnerability. Basically the worst kind of vulnerability. I have a vague memory of at least 1 xss issue in the past that I can't remember in any more detail.

mile23’s picture

Kewlio. https://nvd.nist.gov/vuln/detail/CVE-2017-9841

It says "Util/PHP/eval-stdin.php in PHPUnit before 4.8.28 and 5.x before 5.6.3" which doesn't apply to us as of core 8.8.x. But still, who knows what else lurks in your exposed vendor directory.

Previous mitigation: #2508591: vendor/ is web accessible

Current efforts from Composer intiative: #3057094: Add Composer vendor/ hardening plugin to core

I think this issue is more about core's tests that live in core/tests/.

greggles’s picture

Yes, I mention that as an example of a group of issues that can crop up when development/test code is included in the code that people run on their websites. We have mitigations for it that are imperfect and we can improve those. But this is issue proposes a good step toward avoiding the class of problems.

greggles’s picture

#2946280-8: eval-stdin.php flagged as malware Gives a different but closely related example of test files in webroot causing a site to be abused for spam.

effulgentsia’s picture

By the way, in case anyone doesn't know, per #17, Symfony, and some of our other vendor libraries, do not remove their tests from their tarballs / zip files, but we remove those libraries' tests from Drupal core via these lines in Drupal's root composer.json file:

"post-package-install": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
"post-package-update": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",

That still means that bandwidth is being wasted on downloading Symfony (and other) test files when installing Drupal from composer, but at least those files don't end up on Drupal production sites or in Drupal tarballs.

Not sure if that's helpful or sparks any ideas; just sharing how we as a consumer of Symfony are dealing with Symfony's choice to includes test files in their release packages.

catch’s picture

composer patches doesn't enforce git clones, it it just a mechanism for downloading a patch and applying it to whatever composer download, whether it's from a tarball or git.

So this would mean that if you're using a core patch from Drupal.org, it would cease to apply to the dist version of core - you'd have to modify the patch to remove all the hunks that touch test files. As someone who uses composer patches a lot, this would be very inconvenient.

Also composer patches has nothing to do with core development, it's just a way to manage patches for a development or production site, so anyone who has a composer-based site and finds they need a core or contrib patch would run into this, not only people working on the patches.

greggles’s picture

@catch That's been discussed a bit. See #5 point #4, comment #18, and comment #20. I'm not sure that composer-patches does prefer-source on the target package by default as has been stated here. If it does then that seems to really mitigate that concern. It would be good to definitively verify that.

I think the biggest blocker to this is the concern from #22 that this could raise load on d.o infrastructure.

zaporylie’s picture

Just a note: Symfony decided to remove tests from dist version of their packages in https://github.com/symfony/symfony/pull/33579

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

josephdpurcell’s picture

Regarding the question of whether composer patches does prefer-source, I have done a test with webforms, see https://www.drupal.org/project/webform/issues/3093934#comment-13350022. Instructions are in the referenced webforms comment if you wish to experiment for yourself.

The conclusion I found:

If tests are excluded using a gitattributes file, and you want to apply a patch using composer patches that has changes to a file in the tests directory, then it will fail, unless you specified to install the package from source.

traviscarden’s picture

Thanks for doing that and documenting the process so well, @josephdpurcell!

adamps’s picture

I can see people have presented several goals of this issue:

  1. Security
  2. Reduce disk space on local server
  3. Reduce download size, hence reduce the load on Drupal.org servers
  4. It's common to apply patches even on live sites so it must still be possible to apply patches that might make changes to test code

Perhaps the comments haven't stated it clearly yet but it seems that if we fail to achieve (4) it is a disaster. People will be forced to prefer source and then we don't achieve any of the goals plus as explained in #22, prefer source is much worse for the server load.

Therefore it's seems that we have to find a way for patching to work with test files missing. I'm no composer expert, but it feels like this should be feasible. Looking at cweagans/composer-patches, it seems to use git apply which has an --exclude option. So could we create an enhanced version of cweagans/composer-patches that reads .gitattributes if present and passes the values through to git?

Thoughts anyone??

josephdpurcell’s picture

Great thoughts @AdamPS! Regarding:

> So could we create an enhanced version of cweagans/composer-patches that reads .gitattributes if present and passes the values through to git?

I really like that idea and on the surface it seems possible. A quick look at https://github.com/cweagans/composer-patches/blob/master/src/Plugin/Patc... the getAndApplyPatch() method seems to handle it. It looks like there's a fallback to patch if git apply fails, and I don't see an exclude option with patch in the man page. Let's assume we could make something work for a moment and ask a higher level question:

Is there a case where you would want composer patches to apply a patch to a directory or file that is specified in the .gitattributes file as export-ignore?

mglaman’s picture

Chiming in about tests. I don't mind if core's tests are removed. But, I extend core's base test classes and need the test suite definition.

+++ b/.gitattributes
@@ -59,3 +59,10 @@
+core/tests                      export-ignore

Change this to the following and I'd be happy. Otherwise all of my tests and TDD approach to work would be ruined. Yes, even client work.

/core/tests/Drupal
/core/tests/fixtures

EDIT: nope, that wouldn't work.

See web/core/tests/Drupal/KernelTests/Core/Entity/EntityKernelTestBase.php

That would get excluded.

EDIT 2: what about module test base classes? I extend Migrate's test base for our various import tests.

mglaman’s picture

Status: Needs review » Needs work

Per my thoughts in #37 I'm kicking to needs work. This would completely break all of my projects as PHPUnit would be broken -- no test suites, no base test classes.

mile23’s picture

So right now, we have the vendor hardening Composer plugin. You can read about it here: https://www.drupal.org/docs/develop/using-composer/using-drupals-vendor-...

I haven't tried, but it might be configured to remove test directories from the drupal/core and drupal/[core-module] and drupal/[module] packages.

I'm not entirely sure if it will find the proper directories, given that those packages are installed using composer/installers. If not, we can make a child issue here to allow it to do that.

We'd also need a policy on whether our Composer project templates would include this configuration by default. That would allow the workaround of just removing that config if you need the test stuff, or alternately creating your own Composer project template pre-configured the way you like it.

I still think that the best and most useful solution is to do all the things required to make all the different core testing frameworks into their own packages. Then, instead of worrying about this configuration, you could just say composer require --dev drupal/core-build-test-framework drupal/core-run-tests-runner, and for production: composer install --no-dev.

And, to top it all off: If you use drupal/recommended-project to build your Drupal site with a docroot that does not contain vendor, you're much better off in this regard in the first place.

cweagans’s picture

.gitattributes support seems like a great thing to have, especially if it means not breaking everyone's workflow, so I wouldn't be opposed to making git apply the default and only method for applying patches if that helps.

For the same functionality with patch, you'd have to use something like filterdiff. I'm not sure how widely filterdiff is installed though. Anecdotally, my macOS machine has patch but not filterdiff, so this functionality wouldn't work out of the box for me. I suppose composer-patches could try the fallback only if both patch and filterdiff are installed, and then try to parse .gitattributes or some config value in composer.json to control which hunks get stripped out.

Alternatively, composer-patches could have some kind of preprocess step for patches where an external plugin could have a chance to modify the patches before they're applied. This could be another plugin that Drupal core (and Symfony projects) require, and all it would need to do is strip out hunks related to test files.

Anyway, happy to help. Let me know which way makes more sense for Drupal. This is a pretty good time to make changes like that since 2.0.0 isn't out the door yet. Breaking changes to composer-patches are definitely possible at this point.

adamps’s picture

Issue summary: View changes

OK I updated the IS to include recent comments.

mile23’s picture

Work happening in #3108262: Support directories other than vendor in the Hardening Plugin

This would allow users to configure which core directories to remove during composer install.

chi’s picture

Besides actual tests those directory contain Drupal testing framework (base classes, traits, helper modules, fixtures, etc.). So that straightforward excluding tests directories from drupal/core package will break tests in custom and contrib projects won't it?

Distinguishing Drupal Core tests and Drupal testing framework based on file paths could be tricky because they are closely intertwined.

chi’s picture

Issue summary: View changes

Some other benefits of not having tests in the project code base by default.

  • IDE will index a project faster
  • Fewer irrelevant references in IDE autocompletion
  • Fewer irrelevant references in grep output
  • Faster `git clone` on non-composer projects
chi’s picture

Issue summary: View changes

Added #14 as "Alternative resolution 3".

peterx’s picture

Separating the underlying code from the actual tests sounds like a good move no matter what the other plans are. Similar to separating code and data. Separation of concern and all that good design stuff. Given that PHPUnit is the future, does PHPUnit have a structure/convention we could use for Simpletest?

peterx’s picture

The majority of comments here are from people who develop PHP code. The majority of Web sites are built by people who only develop themes. Twig was added to separate theme development from all the template code, removing the need to test PHP code after theme changes. And then there are all the sites installed through Sympatico style host scripts with no option to change code. A DruLite download is aimed at people not developing PHP code.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

grimreaper’s picture

Hello,

I have recently read this article https://php.watch/articles/composer-gitattributes, and now knowing the export-ignore feature from .git attributes I asked if something similar was possible/recommended for Drupal projects. @andypost gave me a link to this issue on Slack (thanks again).

I have read the issue summary and all the comments. I don't know what is the trend. So if this can help, this is my opinion and usage of Drupal as a developer and contributor.

Usage:

I use Composer on almost every Drupal projects I do or am involved in, only a very few special/small missions are not using Composer.

I have patches applied with cweagan/composer-patches on (almost) every projects on core or other contrib projects.

I prefer using tagged releases. Only when a contrib project does not have a tagged version or some patches are impossible to apply and a fix had been commited in dev version, then I use a dev version and still using a commit hash in composer.json to ensure to not move involuntarily with a composer update. Also tagged releases allows to download translations.

For client/personal projects, even when not doing automated tests, I would like to be able to see the tests because sometimes it helps to see how things work (JSON:API tests are really helpful) or to have real examples on how to test some particular stuff.

In my local setup, I have a dedicated stack/skeleton like the one used for client projects, but this one is for contribution.

And I have a "contrib" folder at the root of this setup where I clone the projects I want/need to make/update patch for (Drupal core, Drush, Composer plugins, any Drupal contrib projects).

So when I need to test stuff, I rsync code from this "contrib" into the appropriate place (vendor for Composer plugins, app/core for Drupal core, app/modules/contrib for ... etc.). That way I am sure that in any case Composer will not remove the cloned repositories I have and all my local work that could be lost.

Opinion:

I don't want to have to use a dev version in Composer (and all the cloning slowness associated) to see the tests or be able to execute it.

I completely agree with the opinion that people downloading Drupal using tar/zip archives are not people interested in tests. So not having it in those archives is perfectly ok IMHO.

I also agree that tests code should be removed for production environments (primary topic I searched this subject for). And if this will be too complicated with .gitattributes or other stuff, it can be delegated to package building tools per project at the preference of the team on the project. Even if, in that case, the network gains for Drupal.org infrastructure will not be present.

That's why I think the approach 3, a require-dev package containing Core tests is the best. I don't know if this could be extended (automatically) for other contrib projects.

Also I may be missing some aspects of the problem.

That's my 2 cents.

Cheers,

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mile23’s picture

I'm in favor of more subtree splits, structured as follows:

Each testing framework becomes a component.

drupal/drupal gets a path repo for each testing framework.

Frameworks are required in drupal/core's require-dev.

The vendor hardening config for drupal/core is updated to delete tests (but not frameworks, since they are now components).

The vendor hardening plugin is updated to understand the difference between require and require-dev. That is, if you say composer install --no-dev then the hardening occurs, but if you say composer install then it does not. This is maybe not the perfect solution, because it means we don't default to security.

geek-merlin’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Proposal #1 seems good to me - it's completely reasonable to require people running tests to use prefer-source.

Composer patches was brought up, but now that we're using MR diffs on d.o everywhere, it's not recommended to link to the online MR diff anyway for security reasons, instead people should commit a local copy of the patch (or use one uploaded to the issue in some cases). When preparing that patch, it's fine to delete any changes to test files. This also makes it more likely that the patch will apply longer if unrelated changes to test coverage are committed to the project too, sometimes I do it anyway for that reason, especially backporting something for a previous minor/major release.

nicxvan’s picture

joachim’s picture

> Composer patches was brought up, but now that we're using MR diffs on d.o everywhere, it's not recommended to link to the online MR diff anyway for security reasons, instead people should commit a local copy of the patch (or use one uploaded to the issue in some cases). When preparing that patch, it's fine to delete any changes to test files.

That's still going to be fairly fiddly to do.

You're either manually poking around in a .patch file removing bits.

Or you need the git skills to revert portions of a branch, and that's fiddly too -- something like:

> git checkout 11.x -- */test/* [maybe? will wildcards work here?]

and then commit the result to your own local branch. But then if the original MR is updated, you have to repeat that process all over again.

I mean, sure, I often have local branches in my clone of core for backporting MRs to a stable version. But I'm not sure we can expect everybody to do that.

The best bet would be a preprocessor to composer-patches, as has been suggested, but I don't know how close a 2.0 release is of that.

kingdutch’s picture

I'm assuming a stance that if people want to use patches of unsupported (non-released) changes in their own project they can be expected to make their own patches. In that case, it's built in git functionality to to exclude files with git's powerful path matching syntax. That works in diff, log and format-patch to name a few of the commands that use the same underlying features.

As an example for a recent Drupal core commit (using --stat to limit the output to what's relevant for the discussion):

$ git diff --stat 422099995ecfe78f1e29248374fc05cd4a556c1d^!   

core/modules/package_manager/src/ExecutableFinder.php                | 22 ++++++++++++++++++++--
core/modules/package_manager/tests/src/Unit/ExecutableFinderTest.php | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 2 deletions(-)

$ git diff --stat 422099995ecfe78f1e29248374fc05cd4a556c1d^! -- ':!**/tests/**'
core/modules/package_manager/src/ExecutableFinder.php | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)

Notice how the second diff doesn't include the test files and could be used to create a patch that would work.

catch’s picture

There's also the issue that including test changes in the diff increases the likelihood that the patch won't apply, either in the first place because there are conflicts with the older branch already, or if it's not committed anywhere yet, that test conflicts are introduced in minor versions even when the runtime code change itself would continue to apply. So there are benefits to removing test files already (I often do it if I'm doing a local backport of a core MR diff already partly for this reason).

alexpott’s picture

For me this is fantastic idea which we should definitely do. But given the disruption to patches I'd only do this in a major release so minor updates to sites that have patches applied don't break just because we've removed tests.

catch’s picture

I bumped this because of #3533965: Package Manager actions intermittently blocked on shared hosting but also now think #3534109: Permanently maintain a sandbox directory in package_manager is probably more urgent for package_manager/Drupal CMS. If the other issue works, then this issue would be a very nice to have on top (and for the other reasons it would be good), and starting in Drupal 12 seems reasonable, although if we somehow miss Drupal 12 then I'm not sure I'd want to wait until Drupal 13, could be Drupal 12.1 with a PSA since it's still early in the major release cycle.

meeni_dhobale made their first commit to this issue’s fork.

catch’s picture

Bumping this to critical and tagging with 'Drupal CMS release target' - we're fairly sure that doing this issue will significantly improve (although not necessarily fix) the problem of package manager sandbox creation hitting shared hosting abuse limits. If we have less files to copy around, there are... less files to copy around.

I'm also removing the RM and FM tags because I discussed this with @alexpott and @xjm at DrupalCon and they were both +1.

The actual commit would need to happen again 12.x when it opens, but we can have something ready to go before then.

edit: thanks to @pameela for demoing how this goes wrong at DrupalCon - entire shared hosting account got locked down and it requires a support ticket to get it unblocked, not even a rate limit, full account freeze.

catch’s picture

Hmm I think this is probably the right tag for Drupal CMS tracking?

catch’s picture

For #37/#38, I think if we try to be clever about which test files to remove it will be more or less impossible to finish.

However given we're only going to start this in a major release, we can probably do one of the following:

1. For running tests locally, or CI, require sites to use --prefer-source prior to running tests.

2. Have another (probably new) package like core-with-tests that doesn't exclude them - but only if #1 is a non-starter.

We also need to make sure this will work for contrib and gitlab.

xjm’s picture

I was debating to myself whether this is something we should do only in a major versus whether it is allowable in a minor. Technically, there is an argument that this is partly allowable in a minor release, because neither tests nor file locations are public API. So long as the autoloader (and GitLab) can still properly serve the testing framework and its base classes and traits for CI, we could do parts of this in 11.3.

This does make it more disruptive than simply the files not being in the packages:

It's common to apply patches even on live sites. Patches often contain changes to test code and would by default fail if it is missing (see #33. Therefore as a dependency of this issue, we need an enhancement to the patching mechanism to allow applying patches when test code is missing - see this issue.

For me, the value of unblocking Package Manager is still worth breaking almost every composer-patches in a minor release, and we should consider whether this could be committed before 11.3.0-beta1.

In practice, we are very close to 11.3.0-beta1 (about 3 weeks away), and the opening of 12.0.x is the next thing to happen after that anyway. Making it a major-only change might be nicer for site owners (or anyone with weird CI/CD that relies on release packages), and I can't think of any practical reason to offer parity on the maintenance minor branch for the change. It's not really something that would need to be part of the continuous upgrade path.

If there are any clearly minor-safe blockers, we should definitely try to land those for 11.3.

xjm’s picture

xjm’s picture

Issue tags: +Drupal 12 beta blocker

The first one!

xjm’s picture

Reparenting. I've listed it on both metas because we should do this in 12.0.x basically as soon as the branch opens, but we must do this prior to 12.0.0-beta1.

catch’s picture

Issue tags: +Drupal 12.0.0 release priority

Adding the 12.0.0 release priority tag because we're likely to use that for other issues too.

quietone’s picture

Issue tags: -Drupal 12.0.0 release priority +12.0.0 release target

Reviewing the use of 'release priority' and 'release target' I think the intention is that this is a 'target'. 'Priority' has been used for things not directly important for the version about to be released. I've seen it used for a broader range of issue, such as long term strategy issues. Of course, there sometimes a mix but that is the trend I see. 'Target' is more urgent than the 'priority' ones.

quietone’s picture

Issue tags: -Drupal 12 beta blocker, -12.0.0 release target +12.0.0 beta blocker, +12.0.0 release priority

Discussed tags in committer slack and updating based on that discussion

quietone’s picture

Issue tags: -12.0.0 release priority +12.0.0 release priority

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

longwave’s picture

To me, proposal 3 is the best solution, because it solves the problem while remaining a trivial change for users who want to include tests (composer require --dev drupal/core-tests) but which also reduces the risks on DA infrastructure (git clones are much more expensive than zip file downloads)

However I don't think it solves the patches problem unless composer/installers or the scaffold plugin also can be convinced to reconstruct the original structure once drupal/core-tests is installed.

I had a quick look at the subtree split tool that is currently used for components - https://bitbucket.org/drupalorg-infrastructure/subtree-splitter which in turn mostly depends on https://github.com/splitsh/lite

splitsh-lite does actually support multiple prefixes in the split so it seems technically possible to find all tests directories, include them in the drupal/core-tests split, and exclude them from drupal/core. At the moment subtree-splitter doesn't support anything close to this but it seems like it would at least be possible if anyone wants to attempt it.

longwave’s picture

I basically share the same concerns as #37 regarding test infrastructure. If the subtree split is too much effort, maybe we can use .gitattributes to only exclude **/*Test.php (concrete test classes) and core/modules/*/tests/modules (test modules)? This would leave behind all other test infrastructure - base classes, traits, etc. - which might be enough for most custom/contrib tests to live with?

longwave’s picture

Status: Needs work » Needs review

Opened an MR with the idea from #79.

berdir’s picture

Add me to the list of people concerned about this I guess. In regards to test modules, something like entity_test is almost an API with lots of contrib using entity types defined by it and we even provided BC for some functions it includes. Another example is redis, which extends a number of core tests. I'm aware of the BC risks, but it allows me to ensure that the altenative implementations of the lock, queue and similar systems are compatible with core expectations, also as core makes changes.

Just throwing in a random idea, didn't read all of this. What if drupal/core-tests (or drupal/core-with-tests then) wouldn't be *just* the tests that would need to be spliced into their original places, but the whole package and comes with an instruction that it replaces drupal/core? That should continue to work with anything that requires drupal/core.

catch’s picture

What if drupal/core-tests (or drupal/core-with-tests then) wouldn't be *just* the tests that would need to be spliced into their original places, but the whole package and comes with an instruction that it replaces drupal/core?

I think this would be the simplest thing if we could make it work. That way we can remove as much as possible from drupal/core, but provide it all again.

Not sure what the composer command would look like to switch between the two on CI though, would be great if just specifying drupal/core-with-tests in require-dev was enough.

longwave’s picture

If we could do #82 with drupal/core-dev then maybe this is achievable with no changes at all for most users!

amateescu’s picture

Just another data point.. the WSE module also heavily relies on extending core test classes for checking the same assertions in the context of a workspace :)

rosk0’s picture

I believe that approach #1 is still the best one - core and every contrib project declare what should be omitted from export by Git. As Git export is used only to produce artifacts for distribution, e.g. for running in production, that is how it should be.

And in my opinion using --prefer-source for CI testing is fine. Composer cache can help to speed up process.

The one degraded experience is the patching and is important of course , but it has a workaround - manual patch amendment to remove test data from the patch, and I consider this lesser evil than delivering all the test data to production environments that is happening right now.

murz’s picture

Probably it's a good idea to exclude the docs from the module packaging too. For example, the AI module in the "docs" directory here https://git.drupalcode.org/project/ai/-/tree/2.0.x/docs?ref_type=heads provides 170+ files that are usually not needed for end users at all. Actually, they are required to only generate the documentation pages here https://project.pages.drupalcode.org/ai/

mxr576’s picture

Probably it's a good idea to exclude the docs from the module packaging too

Make sense, actually all my projects on D.o has similar extra configuration in .gitattributes: https://git.drupalcode.org/project/easy_encryption/-/blob/1.0.0-rc7/.git...

acbramley’s picture

@catch @berdir composer has a notion of a package replacing another, done in the package's own composer.json file

https://getcomposer.org/doc/04-schema.md#replace

Could we have drupal/core-with-tests required by CI and that would just use replace for Drupal/core?

catch’s picture

@acbramley yes I think this would be best if we can make it work.

catch’s picture

Status: Needs review » Needs work

We should add the latest idea of a core-with-tesrs to the issue summary and also try to implement it. Moving to needs work for that.

xjm’s picture

Issue tags: +DrupalSouth 2026
acbramley’s picture

Issue summary: View changes

Added Alternative resolution 4

I'm not really sure what the first step would be to a) strip stuff from drupal/core and also b) setup the drupal/core-with-tests package (name pending)

georgebc’s picture

+1 to the @acbramley 's suggestion.

catch’s picture

On #93 I've been wondering about that too but didn't get very far.

If we can add a subtree split for drupal/core-with-tests (maybe core-tests-included would work?) first, then we could do a full end-to-end test of replaces for drupal/core. Not sure how much we can do without going through the whole process including getting it on packagist to know whether it will really work or not. Even if we found some kind of fatal flaw with this approach and it's already on packagist we can quickly deprecate and abandon it before anyone uses it.

Then the test removal would be a much bigger change to make but should be more self-contained.

longwave’s picture

For option 1: regarding the .gitattributes method will this work with the subtree split? It would likely apply to drupal/drupal but drupal/core is a subtree split of the core directory. Would we need another .gitattributes in the core directory?

For option 4: I think this requires coordinated work in both core (to set up the Composer metadata) and in the subtree splitter (because it doesn't quite support this yet). I think the changes on both sides would be relatively small, but similar to #95 I don't know how we would test them. I think we could even just reuse the drupal/core-dev package name - instead of being a metapackage it would actually pull in core+tests and replace drupal/core.

catch’s picture

Using core-dev is definitely worth a try!

drumm’s picture

Would we need another .gitattributes in the core directory?

That sounds right, and shouldn’t need changes to the subtree splitter. That becomes the .gitattributes file for the core subtree split, which is downloaded from https://api.github.com/repos/drupal/core/zipball/16e55d807cc9f839d281988.... Everything I see says that uses .gitattributes That makes this approach straightforward.

longwave’s picture

Status: Needs work » Needs review

Implemented option 1 with the change from #96/98.

For option 4 I think we need option 1 anyway, so we could land this here and then continue with option 4 in a followup?

longwave’s picture

Status: Needs review » Needs work

One of the build tests is broken.

pameeela’s picture

Issue tags: -Drupal CMS dependency critical, -12.0.0 beta blocker +2.0.0 beta blocker

Discussed with @quietone and @xjm at DrupalSouth, and I am removing the Drupal CMS tag. We are no longer chasing shared hosting for Drupal CMS, and even if we were I don't think this has enough of an impact to change that equation significantly.

So I think this is still worth doing but there's no need to consider the impact on Drupal CMS as far as I'm concerned. Maybe @phenaproxima would disagree, but he's on leave for another few days :)

nicxvan’s picture

Issue tags: -2.0.0 beta blocker +12.0.0 beta blocker

Fixing the version tag.

I agree this is worth doing still.

This is definitely still worth it for shared housing with package manager and automatic updates regardless of cms targets.

It's also good for cleaner production apps and smaller deployments.

This is also good for the issue to move tests to better locations.

macsim’s picture

In my humble opinion, solution 4 (drupal/core-with-tests) is a good answer for developers who need tests locally, but it doesn't fully solve the patching problem: site maintainers using drupal/core dist who want to apply a patch that includes tests still have to either switch packages or use --prefer-source. That's a non-trivial downstream burden.

#61 hints at a complementary approach. The same .gitattributes export-ignore rules could drive a CI step on Drupal.org that generates a dist-compatible patch variant as an artifact alongside the full one. Contributors keep writing tests, composer-patches users get a working patch, no downstream changes needed. And since contrib projects share the same CI infrastructure, the same pattern could extend there naturally.

cmlara’s picture

Issue summary: View changes

We can expect Contrib to follow core in stripping test files out (I already have it on my roadmap for all my modules regardless using option 1).

I would suggest that any method be chosen be one that Contrib can also utilize to promote a unified method throughout the ecosystem.

Proposal 3 (subtree split) and 4 (separate package for core + tests) are (to my knowledge) not possible for Contrib based on current Infra. I would suggest excluding them as options.

Patching:

It has been previously noted git apply --exclude= would likely solve the majority of these issues.

https://github.com/cweagans/composer-patches/issues/295 was closed (it suggestion was to implement the git apply --exclude) however my understanding is that Composer-patches (^2) now supports creating custom patchers, the community would no longer need composer-patches to implement this in their codebase.

At a minimal the freeform patcher (with a shell script) should be able to implement stripping off files in common test folders. https://docs.cweagans.net/composer-patches/usage/freeform-patcher/.

Above should be the vast majority of deployments, where only those without git would need to use regular patch.

Standard *nix patch does not have an exclude option, however this could be paired with a CLI helper script to modify the patch files for those who need it.

catch’s picture

For composer patches we already strongly recommend local patch files instead of an MR diff because the MR diff is non-stable. It's trivial to strip test changes in local patch files and I do it routinely on projects to lower the chances of conflicts anyway. Follow-up to add a custom patcher sounds good though.

mstrelan’s picture

StatusFileSize
new2.07 KB

FWIW I've attached a script you can use to get a local patch from the URL of an MR that filters out tests and phpstan baseline changes. If there are other patterns that need excluding you can add to skip_patterns in the script. In theory this could be extended to also integrate with composer patches so you could run one command that generates the patch and adds it to your composer patches config.

longwave’s picture

Instead of a freeform patcher it also looks possible for a Composer plugin to register a new global patcher that could automatically skip parts of patches that cover tests.

Option 1 is the simplest that doesn't require changes to the packaging scripts or restructuring of the repository. I think this is the easiest option to land but we should also try to help out users who patch core by providing a plugin if we can.

catch’s picture

I think we should start with option 1 and then open a follow-up issue for... option 1A which is making drupal/core-dev core with tests.

The advantage of the core-dev approach is that it would probably transparently work for project-specific pipelines without them needing to use prefer-source, but it feels like a nice to have rather than a blocker. As does a test excluding patcher.

Since we're only going to change this for Drupal 12 even if sites end up having to adjust pipelines and update their patches it will be part of the major upgrade process anyway.

I do think we need to do this asap so that we can iron out any unexpected things that come up sooner than later.

longwave-bot made their first commit to this issue’s fork.

catch’s picture

Issue summary: View changes
longwave’s picture

Status: Needs work » Needs review

Rebased and fixed the build test. The required change to the test helps to prove that test modules will not be shipped with a dist package.

godotislate’s picture

There's an open comment on the MR from @alexpott.

longwave’s picture

Yep, tightened up the pattern. Makes no difference to the dist export, but I agree we might hit false positives in the future.

godotislate’s picture

Status: Needs review » Reviewed & tested by the community

lgtm

  • catch committed 5341dd83 on main
    task: #3067979 Exclude test files from release packages
    
    By:...
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs change record +Needs followup

Let's get this in so we have the maximum amount of time to uncover any unexpected issues and also to sort out the followup mitigations if we think they're necessary.

I think we need follow-ups for:

1. A custom composer patcher that always excludes tests
2. Whether drupal/core-dev should replace Drupal core so that it provides a way to get tarballs-with-tests

I created a change record, moving to needs work for the follow-ups so they don't get forgotten. Committed/pushed to main, thanks!

Did my best with commit credit but this is a very long issue so apologies if I missed anyone.

mably’s picture

[DELETED]

  • catch committed 34b130bd on main
    Revert "task: #3067979 Exclude test files from release packages"
    
    This...
catch’s picture

A link to a pipeline with 'this broke contrib testing against main' would really have been enough here, not sure the Claude output added anything at all.

Pushed a revert to main, I think we need a gitlab templates issue to switch to --prefer-source, but that in turn might need something like https://git.drupalcode.org/project/gitlab_templates/-/work_items/3464400 or we'll need to reconfigure drupal/core-dev after all.

mably’s picture

EDIT: Previous comment deleted as asked.

cmlara’s picture

but the templates generate the project composer.json themselves, so a project cannot set it today.

The template merges in a copy of the module provided composer.json with some defaults.

You should have been able to write this to your own module while waiting on gitlab_templates and had it merged in with their other defaults.

Pushed a revert to main, I think we need a gitlab templates issue to switch to

Not sure the revert was needed since this was always known.
Anyone following next major is expecting their jobs to fail frequently, gitlab_templates should have been able to work around this in minutes with an update to their default composer.json. Having the error present would help them test a fix.

Gitlab_templates does exclude the .git folder from the vendor dirs so the source code should be a non-issue in taking up extra size on the saved archive.

marcelovani’s picture

The commit that landed this morning takes FunctionalJavascript tests with it,
for core and contrib alike. I don't think that specific consequence has come up
yet — the concerns above are about extending core's base test classes, and the
negation keeps those:

modules/*/tests/src/ -export-ignore

but the test *modules* those base classes install are gone. WebDriverTestBase
installs js_testing_ajax_request_test, js_testing_log_test and
css_disable_transitions_test, so every Javascript test now dies in setUp with:

Unable to install modules ... due to missing modules
js_testing_ajax_request_test, js_testing_log_test, css_disable_transitions_test

I hit it on a contrib module whose next-major job was green yesterday and red
today. Reproduced by updating a Drupal 12 build to dev-main: afterwards there
are zero */tests/modules directories anywhere under web/core — user, node,
system, all of them — while core's git tree still has them. The same tests pass
against a core checkout from before the commit.

So as it stands, a --prefer-dist install of core cannot run Javascript tests at
all, which I think makes this a fair bit wider than the patching concern raised
earlier.

mstrelan’s picture

Can we stop letting AI write our comments please? Fine to use it for debugging or working on an issue, but understand the output and communicate with us humans with human language.

longwave’s picture

Status: Needs work » Needs review

Carved out some more exceptions that should help with contrib tests. Maybe we should move those three helper modules to some other directory?

Also trying to figure out how to write a test that proves that tests can still run, but this is tricky - I think it needs to be a build test that can then execute a test inside that. But this will be useful both to prove that this works and also prevent regressions in the future.

cmlara’s picture

Here is a sample of what a contrib module (or gitlab_templates) would put in their composer.json (Originally mentioned in comment 20 as a better options than --prefer-source).

{
    "config": {
        "preferred-install": {
            "drupal/core": "source",
            "*": "dist"
        }
    }
}

That would allow core to strip out ALL tests classes as called for in solution 1, including the base classes.

mstrelan’s picture

If I understand #126 correctly, that would mean all custom code running their own tests extending from base classes would need this config, not just contrib.

cmlara’s picture

Correct.

I thought all the post before were contending was the plan (I didn't look closely at the MR, just thought it included anything test related based on what I saw). I did see the IS talking about maybe excluding framework from the ignore however didn't realize there was actual intention to follow through with that as it wasn't listed as part of the selected option).

Having ecosystem require-source for Core to run Core or contrib, or whatever custom tests you have seems like a small cost to not have to always ask "will this break 3rd party tests if they don't require-source for Core". Documentation wise, that is no different then telling developers they need to require core-dev, its just an environment setup step.

I would not try and keep code like WebDriverTestBase (or KernelTestBase or FunctionalTestBase, etc) without source. I gurantee if Core does so there are going to be incidents where a developer and Core committer don't realize something that is add to the Base class is excluded from the packaged release and there is now an emergency hotfix required just for development code to fix pipelines globally.

mstrelan’s picture

The issue I have with the composer config in #126 is our production build artifacts are generated from the same composer.json, and we probably don't want to install from source there. I guess in that case we can use --prefer-dist to override this. I think the drupal/core-dev approach is simpler as we don't have to adjust any config or build steps.

nicxvan’s picture

@mably, yes please clean that up, it's quite a bit to scroll past.

longwave’s picture

Added a build test that creates an exported test site and runs trivial Functional and FunctionalJavaScript tests to prove that tests can at least be executed end to end with .gitattributes in place. This will also catch some regressions if we alter the test base classes in the future.

This test was written by GPT 5.6.

mably’s picture

@longwave we are using Drupal\FunctionalJavascriptTests\PerformanceTestBase, is it included?

I guess most test Base classes should be kept available.

spiderman’s picture

@star-szr and I have been tracking this ticket, and are excited to see the movement this week! We wanted to weigh in on a couple points to try and draw out what remains to finish this off.

In reviewing the discussion overall, option 1A and 4 appear to us to be the same, with the possible exception of overloading the existing `core-dev` package. It's not entirely clear what was intended here, but we would like to advocate for leaving `core-dev` as-is, for the purpose it was intended for, rather than overload this use-case onto it. In other words, if we're introducing a new package, let's call it something like `drupal/core-with-tests`.

We're not familiar with the specifics of the pipeline that builds core packages, but adding a `drupal/core-with-tests` package would seem to entail a relatively small change from the existing pipeline. We'd need to remove the .gitattributes that ignores test files, and probably a small change to the composer.json to add the "replace" key into the new package. Does that make sense?

longwave’s picture

To me option 1 is currently my preference, and any users who actually need all core tests should download source instead. I believe this is the intended spirit of both the .gitattributes and Composer prefer-source features.

Option 2 doesn't solve any of our problems other than disk space on end user servers, so I think we should rule this out. Similarly option 3 would require a significant refactoring of core to be able to cleanly split directories; Composer does not allow packages to be overlaid on each other as far as I know, so this can likely also be ruled out.

Option 1 and option 4 both run into the composer patching problem for dist users - just that the solution is slightly different; option 1 users need to download source, option 4 users need to add a separate package.

Option 4 is more work for core maintainers and the DA as we have to update the subtree splitter to be able to create these new packages from a single source repository and also maintain this going forward.

Therefore option 1 is the most straightforward, and we can build a future Composer patcher plugin if it turns out to be a real world problem.

deviantintegral’s picture

Should the change record be updated or unpublished for now since this was reverted?

longwave’s picture

Thanks, done.

star-szr’s picture

@mably could you please share a link to the pipeline that failed after this was committed? Or just provide the project name?

mably’s picture

@star-szr: yoyaku, this job (25 Aug 10:24 UTC) lost 20 classes in setUp() to missing modules js_testing_ajax_request_test, js_testing_log_test, css_disable_transitions_test and performance_test, which WebDriverTestBase and PerformanceTestBase install themselves; the same branch was green again at 12:59 after the revert. Kernel side, kessai gave 134 x Unavailable module: 'entity_test'.

star-szr’s picture

Thanks @mably for sharing that info!

@spiderman and I have been working on trying to reproduce the contrib CI failures and figure out a path forward.

We have been working with a gitlab-ci-local setup to try to reproduce the failures encountered with the 5341dd83 commit. Long story short, there have been many roadblocks along the way and at least from our experience it's non-trivial to test contrib CI with a specific version (commit hash or issue fork) of core.

The general difficulty of testing the .gitattributes approach and the growing list of exceptions seems to call into question the long-term sustainability/maintainability of .gitattributes as a sole solution. Certainly .gitattributes is part of the picture here, but we have some concerns that there is complexity being baked-in here that may not be necessary. See also the last paragraph of #128. There doesn't seem to be a way to fully test the exceptions in the .gitattributes file either, and it seems to water down the security benefits of excluding test code from production artifacts as well.

Tomorrow we are planning to explore the failure that happens when using prefer-source in CI. Barring a solution to the CI problem(s), it seems like we may need to consider creating a core-with-tests package (4 from the issue summary), or if that is really not viable, to change core-dev to no longer be just a metapackage (1A from the issue summary).

longwave’s picture

Re #139 I added test coverage to prove that simple BrowserTestBase and WebDriverTestBase tests can be run on a site exported as per .gitattributes; we can add additional test coverage if we find further cases that this does not cover.

spiderman’s picture

@star-szr and I have continued to work on this today, and came up with a test methodology that allowed us to see the failure @mably reported in CI, using gitlab-ci-local and the .gitattributes patch(es). Specifically, we:

  1. Introduced before_script step which forcibly creates a git archive at the specific TEST_CORE_REPO_REF we specify, and using a URL of our choosing:
    composer (next major):
      variables:
        TEST_CORE_REPO_URL: https://git.drupalcode.org/project/drupal.git
        TEST_CORE_REPO_REF: 5341dd83175be849538fd282559ab3b295e94b7f
      before_script:
        - mkdir -p /opt/core
        - mkdir -p /tmp/drupal-core-git
        - cd /tmp/drupal-core-git
        - git init --initial-branch=main
        - git remote add origin $TEST_CORE_REPO_URL
        - git fetch --depth=2 origin $TEST_CORE_REPO_REF
        - git reset --hard FETCH_HEAD
        - git log --oneline --decorate
        - git show --stat
        - git archive --format=tar --output /tmp/drupal-core-dev-main.tar main:core
        - tar xf /tmp/drupal-core-dev-main.tar --directory /opt/core
    
  2. Adjusted the project's composer.json to include our custom core package, checked out at a specific commit, and having the .gitattributes exclusions applied.
    {
        "repositories": {
            "drupal": {
                "type": "composer",
                "url": "https://packages.drupal.org/8",
                "exclude": [
                    "drupal/core"
                ]
            },
            "drupal/core": {
                "type": "path",
                "url": "/opt/core",
                "options": {
                    "versions": {
                        "drupal/core": "dev-main"
                    },
                    "symlink": false
                }
            }
        }
    }
    

Note: The above .gitlab-ci.yml stanza, along with the following composer.json repository config, will allow you to test against arbitrary commits of core, including issue forks. In addition, you can substitute in the following values to test against other relevant commits.

Commit where the core/.gitattributes change was reverted:

    TEST_CORE_REPO_URL: https://git.drupalcode.org/project/drupal.git
    TEST_CORE_REPO_REF: 34b130bddb6d985b359f38fe993fe77915f96efa

Issue fork for this issue:

    TEST_CORE_REPO_URL: https://git.drupalcode.org/issue/drupal-3067979.git
    TEST_CORE_REPO_REF: 3067979-exclude-test-files

Going further, we used the same methodology to test the latest commit in the issue fork here, to see if the current state of this issue's patch would still cause problems for the Yoyaku project we'd seen failing. In this scenario, we ran into a missing Drupal\Tests\views\Kernel\ViewsKernelTestBase.

Next we extended our before_script to add an extra set of exceptions to leave ViewsKernelTestBase intact, and found that we ended up back at the missing performance_test module. As such, it would seem there is likely a *viable* path forward by adding exceptions and ignores, but this points to the maintainability/sustainability challenge we mentioned yesterday.

It seems to us that the added complexity of using .gitattributes approach (with exceptions to accommodate contrib CI) is a) going to be hard to get correct for all of contrib, and b) neverending, since any contrib maintainer could depend on some new test Trait or Base class that's not in the list yet. Worse, such a maintainer will be stuck without recourse until a core issue to amend the .gitattributes with the exception they need lands.

Beyond that, if we end up re-including a significant amount of the test code we're trying to exclude, it seems like the security, size, and bandwidth goals mentioned in the issue summary are at least partly negated.

The simplicity of adding one file to exclude tests is appealing, but having that file try to carefully ignore the exclusion of *some* test code seems to create more complexity than is warranted. We intend to explore the prefer-source approach a bit further, in hopes that it can mitigate the need to introduce a new core-with-tests package and leave the .gitattributes file relatively short and simple.

spiderman’s picture

@star-szr and I did start looking at the alternative approach briefly today, to see if we could reproduce the issue that had been reported where artifacts were too large to be transferred between stages in the CI environment.

At least locally using gitlab-ci-local, we were able to get Yoyaku's CI pipeline running "normally", without complaints about missing test code (we were still seeing deprecation errors but they appeared to be orthogonal). We used the config snippet from #126 to tell Composer to "prefer-source" the drupal/core package.

For reference, our test methodology here was like so:

  1. Override the `composer (next major)` job in .gitlab-ci.yml:
          composer (next major):
            variables:
              DRUPAL_CORE: dev-main#5341dd83
    
  2. Specify in the project's root composer.json that we want to use the dev-main versions of the core-composer-scaffold and core-recommended:
          "drupal/core-composer-scaffold": "dev-main",
          "drupal/core-recommended": "dev-main",
     

We are planning to create an issue fork of the Yoyaku project to take this a step further and see if the prefer-source approach allows CI for this project to pass using the more complete `.gitattributes` test code exclusions, in case the success we had with gitlab-ci-local is not reproducible on Drupal's GitLab instance. We'll report our results back here, but are cautiously hopeful that this technique may well mitigate the contrib CI challenges.

cmlara’s picture

Status: Needs review » Needs work

since any contrib maintainer could depend on some new test Trait or Base class that's not in the list yet.

Any reason the testing doesn't assert all base classes are present?

More specifically I would expect real testing of this proposal to do as follows:

  1. Identify every Base test class in the repository
  2. Create the export
  3. Ensure every Base class is still in the export
  4. Recursively iterate through every base class use statement to ensure any referenced \Drupal class actually still exists on disk (if any are missing your export is missing a required file)
  5. For every kernel or higher base class ensure that any referenced modules exist on disk(in entirety)
  6. Ensure every file referenced in comments/text/code/string/etc mentioned in every base file exists after export (this is going to be the hard one, and these may also be some of the files you most want to exclude from the export for size)

All the but the 6th item are (relatively) easy to test, that one is going to be hard, and in some ways its also the most important, as it will be the hardest to spot visually in a code review. I've seen fixtures missing in the current export that are called out in base test classes included.

Less than 1-5 and the tests IMHO are just trying to complete a checkbox rather than actually validate the export. Step 3 could be modified to have an exclude list of Base classes that will not be exported, that allows creating new base classes in the future that are source only yet the non-export would appear as part of the merge request as an intentional action.

Back to needs work for my MR comment yesterday regarding the export test as is not testing.

longwave’s picture

Is every base class strictly useful to downstream tests, though?

Also, what about test modules and themes - how do we identify which ones are useful for downstream and which are core only?

catch’s picture

I think I agree with #141 - we should either force everyone to use require source all the time (but this will mean trying to do this in parallel with gitlab_templates updates as well as some documentation), or go with 1A/4 where a dev dependency can bring back the tests. If we have to keep a lot of base classes and test modules in core then it's going to be a list of things to constantly maintain and also mean we're not removing as much code from the release packages as we could.

cmlara’s picture

Is every base class strictly useful to downstream tests, though?

Technically it’s a class that’s suppose to be extended for testing (though as a testing class it is off API). Better to ask why it’s called a base class if it isn’t useful.

I can’t claim to know all of them, most are in areas I don’t need, however if this issue is taking the standpoint of “don’t break existing tests” it has to assume the base classes are in use especially when we already have at least one example above.

As a dev, if even 1% of the modules I work with end up needing prefer-source I’m likely just going to make it my standard template for 100% of my modules as it’s cheaper to solve a problem with standards before it ever occurs.

Even if core doesn’t include every base class steps 3-6 of my comments on testing are still relevant to ensure those core do include are complete.

how do we identify which ones are useful for downstream and which are core only?

I can’t give a general answer, there is certainly room to debate, my comment was that if the module is referenced in a base class that is included in the export that module (test or real) is by definition necessary for the test class to function correctly, if you don’t test all of its features and just run through setup you may not trigger an error. Doesn’t mean an error isn’t hiding in the code.

At one point in my career around 20 years ago I was building install images for custom appliances, pairing them down without missing anything criticial was always painful, and it is essentially what core is trying to do right now. I would be dramatizing to say “if you removed a font file it could cause a segfault, but only on Tuesday’s” , that never happened, however it essentially described what missing dependency incident was like, you often encounter them only when the perfect conditions exist.

For core this trimmed export picking and choosing as little as possible is new and unexperienced territory, for me this is a a hell I’ve already lived and can say even with being excessively conservative on what I trimmed I still had bad days.

spiderman’s picture

Status: Needs work » Needs review

@star-szr and I experimented with the "prefer-source" approach, as discussed yesterday. See our issue and MR on the yoyaku project, for details. We altered the CI pipeline to use "prefer-source" for just drupal/core (in the "composer (next major)" job). This allowed the job (and subsequent phpunit jobs) to complete normally. We didn't see any problem with artifact size limits being hit, which makes sense since the .git history is excluded from the artifacts in CI.

Of note (specific to our testing with Yoyaku), there is a test that seems to always (or frequently?) fail, but this was the case before we made any substantive changes. Specifically, Drupal\Tests\yoyaku_calendar\FunctionalJavascript\SlotBookingPerformanceTest. See our first pipeline without changes on the issue fork MR.

Based on these results, our suggestion would be to incorporate a change like this (composer config --global preferred-install.drupal/core source) into the gitlab_templates project templates. For now, this could apply only to the "composer (next major)" job, but later could be applied more generally. This also seems to obviate the need for an additional "core-with-tests" package to be introduced along with the `.gitattributes` patch.

Further, given "prefer-source" is a viable workaround, it would seem that we could go back to the original, simpler version of `.gitattributes` from @catch's original commit to main. In fact, we might be able to simplify this further to exclude even more test code (such as some of the base classes that were initially allowed), since we know that CI pipelines that need that code can get it all back with "prefer-source".

Specifically, it would appear we could leave out the following exceptions:

modules/*/tests/src/ -export-ignore
profiles/tests/testing/ -export-ignore

Is there a reason to leave the testing profile intact, except for contrib testing?

spiderman’s picture

Status: Needs review » Needs work

Thanks to @star-szr who noticed I had inadvertently flipped the status on this. Putting it back as intended, sorry for the noise :)

catch’s picture