Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding .gitattributes to remove Tests directory from "dist" #33579

Merged
merged 1 commit into from Sep 16, 2019

Conversation

@Nyholm
Copy link
Member

commented Sep 14, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no?
Tickets
License MIT
Doc PR

This is a controversial topic that have been mentioned before. We recently had some discussions on Slack about it and the community not in an agreement. This was asked back in 2014 already.

Im making this PR again, because I think this will help more people than it hurts to keep the tests in the "dist" version.

Reasons for keeping the tests with the source

  • You can look at the tests to understand how the code works
  • It is convenient

In the past there were an argument of people might depend on Symfony's classes in Tests. That is no longer the case since we moved reusable classes from Tests to Test.

Reasons for removing them (merging this PR)

  • There should be difference between composer update --prefer-source and composer update --prefer-dist
  • Smaller packages when deploying with Docker or on Serverless.
  • Static analysis tools will not complain on PHP syntax errors in our tests (example)

How to decide?

Merging this PR or not is tricky because no side has a solid technical argument. It is basically just personal preference. Please give this PR a 馃憤 or 馃憥 if you want to give your opinion.

Other PRs and issues related to this:

Add .gitattributes file (#29277)
Added .gitattributes files to root and all components (#26472)
Exclude non-essential files from Composer package (#25414)
[HttpFoundation] optimize files for distribution (#24427)
Add .gitattributes files (#23926)
[Suggestion] Adding .gitattributes to ignore unnecessary folders and files for production env (#20057)
Add lightweight and root only .gitattributes (#18004)
Add .gitattributes to exclude tests from ZIPs (#17995)
[RFC] Move tests out of the source and source out of the tests (#17749)
Removal of development & testing files using .gitattributes (#16174)
Please add .gitattributes files and fix line endings (#13521)
making use of .gitattributes (#11810)

Workarounds

There are workarounds for both sides. Example:

Workaround if merged

  • composer update --prefer-source

Workaround if closed

@Nyholm Nyholm requested review from dunglas, lyrixx, sroze, xabbuh and yceruto as code owners Sep 14, 2019
@Nyholm Nyholm force-pushed the Nyholm:gitattributes branch from 4b08f6a to b30d617 Sep 14, 2019
@jvasseur

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

Another argument in favor of removing them is that it will prevent people from depending on Symfony tests. Since we don't have a BC policy for them it's a good thing to prevent people from using them (it would prevent issues like #29426).

@fbourigault

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

Could we see the size of all tests and the size of the whole repository?

@Nyholm

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2019

Good question. I gathered some numbers. Tests are about 35% of the file size of the package.

Component Size (B) Test size (B) Percent
src/Symfony/Bundle/DebugBundle 144 16 11%
src/Symfony/Bundle/FrameworkBundle 7888 5176 66%
src/Symfony/Bundle/SecurityBundle 2280 1584 69%
src/Symfony/Bundle/TwigBundle 1048 344 33%
src/Symfony/Bundle/WebProfilerBundle 1336 128 10%
src/Symfony/Bundle/WebServerBundle 192 16 8%
src/Symfony/Component/Asset 272 96 35%
src/Symfony/Component/BrowserKit 416 208 50%
src/Symfony/Component/Cache 1664 688 41%
src/Symfony/Component/Config 1416 568 40%
src/Symfony/Component/Console 3864 2648 69%
src/Symfony/Component/CssSelector 808 320 40%
src/Symfony/Component/Debug 832 480 58%
src/Symfony/Component/DependencyInjection 6472 4392 68%
src/Symfony/Component/DomCrawler 696 392 56%
src/Symfony/Component/Dotenv 168 40 24%
src/Symfony/Component/ErrorHandler 824 504 61%
src/Symfony/Component/ErrorRenderer 528 96 18%
src/Symfony/Component/EventDispatcher 352 136 39%
src/Symfony/Component/ExpressionLanguage 472 184 39%
src/Symfony/Component/Filesystem 312 152 49%
src/Symfony/Component/Finder 576 312 54%
src/Symfony/Component/Form 6048 3208 53%
src/Symfony/Component/HttpClient 872 152 17%
src/Symfony/Component/HttpFoundation 2576 1328 52%
src/Symfony/Component/HttpKernel 3216 1512 47%
src/Symfony/Component/Inflector 96 24 25%
src/Symfony/Component/Intl 33400 1056 3%
src/Symfony/Component/Ldap 432 120 28%
src/Symfony/Component/Lock 568 208 37%
src/Symfony/Component/Mailer 1216 296 24%
src/Symfony/Component/Messenger 2280 984 43%
src/Symfony/Component/Mime 1640 656 40%
src/Symfony/Component/OptionsResolver 424 192 45%
src/Symfony/Component/Process 488 184 38%
src/Symfony/Component/PropertyAccess 608 304 50%
src/Symfony/Component/PropertyInfo 408 200 49%
src/Symfony/Component/Routing 3720 2872 77%
src/Symfony/Component/Security 3760 1384 37%
src/Symfony/Component/Serializer 2248 1160 52%
src/Symfony/Component/Stopwatch 152 40 26%
src/Symfony/Component/Templating 376 144 38%
src/Symfony/Component/Translation 2048 952 46%
src/Symfony/Component/Validator 6208 1896 31%
src/Symfony/Component/VarDumper 1288 408 32%
src/Symfony/Component/VarExporter 472 296 63%
src/Symfony/Component/WebLink 120 32 27%
src/Symfony/Component/Workflow 832 320 38%
src/Symfony/Component/Yaml 2224 544 24%
Total 110.280 38.952 35%

Update 1

I made a mistake when I first posted this. Numbers are correct now. 2019-09-14 17.52 CET

Update 2

If we exclude the Intl component the average is about 49%. So almost half file size of the component is tests.

@Zarcox

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

Finally ! I think open source does lack a real normalized way to exclude files from distributions.

According to me, test files shouldn't be included in packages we ship in production since they won't be of any use there. Just like I wouldn't include documentation files either, which are doomed to permanent obsolescence.

And among all the good arguments I can think of, there's also the fact that your IDE's autocomplete will be free from these being suggested tirelessly 馃憤 !

@ruudk

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

Yes please, this dramatically reduces the size of our packaged Docker deployments. On our large application I discovered that tests inside the vendor directory were responsible for +-30MB. When deploying 10+ times a day this will save bandwidth and deployment times.

@dkarlovi

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

@sstok

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

Please!

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

Here are interesting considerations on the topic I fully agree with and tried (failed) to explain before: sebastianbergmann/diff#84

To me, as the linked issue suggests also, a composer plugin should exists to deal with this and maybe other installation steps. I'm surprised nobody did write one actually given how important this seems to be by looking at the support it gathers.

Note this is not a formal objection against merging this PR: I'm just going to abstain from voting and let the decision to the rest of the core team.

@thib92

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

On the argument of 芦聽users can look at tests to see how it works聽禄, users can also do that on GitHub on the Symfony repo.

Thanks to the code navigation feature, it has become pretty easy to do so!

@nicolas-grekas nicolas-grekas added this to the next milestone Sep 14, 2019
@Pierstoval

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

I agree for all reasons already listed above that are in favor of removing tests from the tarball 馃憤

Another reason is that when you use your IDE to search for a vendor class, you end up having test classes too, which can disturb our search:
image

And by the way, removing 25% of the size of the package itself means 25% less bandwidth consumption. Don't forget we're in an era where bandwidth has a tough cost, and the more we can diminuish it, the better. Can mean nothing for certain people, but I think it's still a valid reason. Especially when Symfony is downloaded thousands of times per day.

@iquito

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

"Polluting" the results in IDEs when searching for usages of a class would be my major factor to exclude tests. I often find that 90% or more of where a class is used in the Symfony directories is in tests, covering up the important results (like the service definitions or actual usages in executed code). In addition to having to download the tests even if you never need them, IDEs also have to analyze them, which takes time, and having them suggested as type hints is never useful. Sure, the IDE could solve much of that with custom features, but it would be much easier to solve it for all IDEs and tools by excluding the tests by default.

@Nyholm Nyholm force-pushed the Nyholm:gitattributes branch from 65f4cd5 to 3d34f0d Sep 15, 2019
.gitattributes Outdated Show resolved Hide resolved
@yceruto yceruto closed this Sep 15, 2019
@yceruto yceruto reopened this Sep 15, 2019
@@ -0,0 +1,11 @@
.appveyor.yml export-ignore

This comment has been minimized.

Copy link
@localheinz

localheinz Sep 15, 2019

Contributor

What do you think about

  • adding trailing slashes to distinguish between files and directories
  • keeping entries also sorted by kind
  • aligning export-ignore

?

diff --git a/.gitattributes b/.gitattributes
index 0abf5af7b4..fa872b5135 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,11 +1,11 @@
-.appveyor.yml export-ignore
-.editorconfig export-ignore
-.composer export-ignore
-.gitattributes export-ignore
-.github export-ignore
-.gitignore export-ignore
-.php_cs.dist export-ignore
-.travis.yml export-ignore
-build export-ignore
-phpunit export-ignore
-phpunit.xml.dist export-ignore
+.composer/          export-ignore
+.github/            export-ignore
+build/              export-ignore
+.appveyor.yml       export-ignore
+.editorconfig       export-ignore
+.gitattributes      export-ignore
+.gitignore          export-ignore
+.php_cs.dist        export-ignore
+.travis.yml         export-ignore
+phpunit             export-ignore
+phpunit.xml.dist    export-ignore

This comment has been minimized.

Copy link
@stof

stof Sep 15, 2019

Member

agree with sorting, but not with aligning, for the same reason we don't align assigments: adding a new longer line would make us edit all lines.

@weshooper

This comment has been minimized.

Copy link

commented Sep 15, 2019

Also reduces CO2 emissions

Almost impossible to calculate, but I had a go while removing tests from Codeception @ Codeception/Codeception#5527 馃

@fabpot fabpot merged commit ac7dc24 into symfony:4.4 Sep 16, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
@Nyholm

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

Thank you for merging

@Nyholm Nyholm deleted the Nyholm:gitattributes branch Sep 16, 2019
@nicholasruunu

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

For anyone who always wants the symfony tests locally, you could add this to your ~/.composer/config.json:

{
    "config": {
		"preferred-install": {
        	"symfony/*": "source",
        	"*": "auto"
		}
    }
}
  • edit: missed "preferred-install" in config.
leofeyer added a commit to contao/contao that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d279 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/calendar-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/comments-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/newsletter-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/faq-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/installation-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/news-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/manager-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/listing-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
leofeyer added a commit to contao/core-bundle that referenced this pull request Sep 20, 2019
Description
-----------

This PR adds a `.gitattributes` file to exclude the tests when installing from dist. The related discussion is here: symfony/symfony#33579

Commits
-------

b31d2792 Do not install the tests with "prefer-dist"
fabpot added a commit to sensiolabs/SensioFrameworkExtraBundle that referenced this pull request Oct 5, 2019
This PR was merged into the 5.4.x-dev branch.

Discussion
----------

add .gitattributes

Since symfony/symfony#33579 was merged I thought it also makes sense for this bundle.

Commits
-------

4020b7b add .gitattributes
fabpot added a commit to twigphp/Twig that referenced this pull request Oct 17, 2019
This PR was squashed before being merged into the 2.x branch (closes #3153).

Discussion
----------

Update .gitattributes to remove tests from "dist"

This was proposed many times before, but considering symfony/symfony#33579 is now merged, I propose this again. I refer to that issue as for the reasoning, as it applies here also.

I did not add the `docs` directory as there was much discussion about that before (discussion in  #942)

Related: #942 #1776 #1981 #2292 #2799 2905

Commits
-------

5cde1f5 Update .gitattributes to remove tests from \"dist\"
symfony-splitter pushed a commit to twigphp/inky-extra that referenced this pull request Oct 17, 2019
This PR was squashed before being merged into the 2.x branch (closes #3153).

Discussion
----------

Update .gitattributes to remove tests from "dist"

This was proposed many times before, but considering symfony/symfony#33579 is now merged, I propose this again. I refer to that issue as for the reasoning, as it applies here also.

I did not add the `docs` directory as there was much discussion about that before (discussion in  #942)

Related: #942 #1776 #1981 #2292 #2799 2905

Commits
-------

5cde1f50 Update .gitattributes to remove tests from \"dist\"
symfony-splitter pushed a commit to twigphp/twig-extra-bundle that referenced this pull request Oct 17, 2019
This PR was squashed before being merged into the 2.x branch (closes #3153).

Discussion
----------

Update .gitattributes to remove tests from "dist"

This was proposed many times before, but considering symfony/symfony#33579 is now merged, I propose this again. I refer to that issue as for the reasoning, as it applies here also.

I did not add the `docs` directory as there was much discussion about that before (discussion in  #942)

Related: #942 #1776 #1981 #2292 #2799 2905

Commits
-------

5cde1f50 Update .gitattributes to remove tests from \"dist\"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.