Adding .gitattributes to remove Tests directory from "dist" #33579
Conversation
4b08f6a to
b30d617
Compare
|
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). |
|
Could we see the size of all tests and the size of the whole repository? |
|
Good question. I gathered some numbers. Tests are about 35% of the file size of the package.
Update 1I made a mistake when I first posted this. Numbers are correct now. 2019-09-14 17.52 CET Update 2If we exclude the Intl component the average is about 49%. So almost half file size of the component is tests. |
|
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 👍 ! |
|
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. |
|
Thread from before |
|
|
|
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. |
|
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! |
|
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: 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. |
|
"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. |
65f4cd5 to
3d34f0d
Compare
.gitattributes
Outdated
| @@ -0,0 +1,11 @@ | |||
| .appveyor.yml export-ignore | |||
There was a problem hiding this comment.
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-ignoreThere was a problem hiding this comment.
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.
|
Also reduces CO2 emissions ✅ Almost impossible to calculate, but I had a go while removing tests from Codeception @ Codeception/Codeception#5527 🤓 |
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"
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"
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"
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"
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
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\"
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\"
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\"
This PR was merged into the 6.2-dev branch. Discussion ---------- Update .gitattributes to remove tests from "dist" | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Doc update? | no | BC breaks? | no | Deprecations? | no | Fixed tickets | #268 #804 #839 | License | MIT 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. Commits ------- e5e0ce0 Update .gitattributes
This PR was squashed before being merged into the 4.4 branch. Discussion ---------- add missing gitattributes for phpunit-bridge | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | | License | MIT | Doc PR | Seems like the phpunit bridge has been forgotten in #33579 Commits ------- d4c052a add missing gitattributes for phpunit-bridge
This PR was squashed before being merged into the 4.4 branch. Discussion ---------- add missing gitattributes for phpunit-bridge | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | | License | MIT | Doc PR | Seems like the phpunit bridge has been forgotten in symfony/symfony#33579 Commits ------- d4c052a2fa add missing gitattributes for phpunit-bridge
This PR was merged into the 6.2-dev branch. Discussion ---------- Update .gitattributes to remove tests from "dist" | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Doc update? | no | BC breaks? | no | Deprecations? | no | Fixed tickets | #268 #804 #839 | License | MIT 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. Commits ------- e5e0ce0 Update .gitattributes
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
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)
composer update --prefer-sourceandcomposer update --prefer-distHow 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-sourceWorkaround if closed
find vendor/symfony -name "Tests" -type d -exec rm -r "{}" \;