Add .gitattributes to exclude tests from ZIPs #17995

nicolas-grekas · 2016-03-03T09:35:12Z

Q	A
Branch	2.3 (no squashing required)
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#17749, #6605, #5674
License	MIT
Doc PR	-

In #6605, @fabpot gives 3 args against .gitattributes export-ignore:

people use some of our test classes in their own tests (we have already moved some of them to a Test/ directory and that helps bu I fear that many more will need to be moved -- see https://twitter.com/huizendveld/status/288331330998116352 for an example);

people like to have a look at the tests to understand how the code works;

when working on a patch locally on a project, there is no way to run the unit tests to check that everything still work as expected.

I think args 1. and 3. do not apply anymore: Test/ directories have been created and we do not support extending/using anything in Tests/. In fact not shipping these dirs is a good way to enforce this. For 3., since we do not want PRs on standalone subtree-splits, we should not support running tests without first checking out the main symfony/symfony.

Argument 2. could remain, but has to be balanced by arguments presented in #17749.

The resulting zip file (as constructed by github) is 4.3Mb whereas it's 5.7Mb on the current 2.3 (-25%).
Note that I'm personally not in favor of moving Tests/ out of each components directory! Most if not all of the benefits mentioned in #17749 should be achieved already here.

I think we should also open a sibling PR for updating all README.md files: we should not tell about how to test there. But linking to each component's doc in turn would be great (as suggested in #17981).

…symfony#5674)" This reverts commit b33d5bc.

stof · 2016-03-03T10:05:23Z

src/Symfony/Bridge/Doctrine/.gitattributes

@@ -0,0 +1,2 @@
+/Tests export-ignore


you should also add .git* export-ignore to ignore the gitattributes and gitignore from the archive

added (also with a check in the phpunit wrapper)

fabpot · 2016-03-03T12:47:36Z

phpunit

+    if (!$runningProcs) {
+        echo "Symfony tests are missing, did you get the code from a ZIP file?\n";
+        echo "Please use git to fetch the source at https://github.com/symfony/symfony\n";
+        exit(1);


fetch the source from?

stof · 2016-03-03T13:28:35Z

phpunit

@@ -119,6 +119,12 @@ if (isset($argv[1]) && 'symfony' === $argv[1]) {
    }
    chdir($oldPwd);

+    if (!$runningProcs) {
+        echo "Symfony tests are missing, did you get the code from a ZIP file?\n";
+        echo "Please use git to fetch the source from https://github.com/symfony/symfony\n";


This should be written to stderr instead

nicolas-grekas · 2016-03-03T17:13:29Z

PR is ready, comments welcomed. The failure on Travis is related to a limitation of composer that eats all memory when a local packages.json defines a lot of local packages, which is the case here where every single component+bridge+bundle is modified so each of them is locally repackaged for per-components tests.

Appveyor failure is unrelated (fixed in #18002)

nicolas-grekas · 2016-03-03T18:31:30Z

Given the 3 other proposals at #17749 (comment) that look way better to me, I convinced myself that this was a bad idea.
Closing :)

This PR was squashed before being merged into the 2.3 branch (closes #17997). Discussion ---------- Updated all the README files | Q | A | ------------- | --- | Branch | 2.3 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Related to #17995. Commits ------- 2e81248 Updated all the README files

nicolas-grekas · 2016-03-04T08:11:51Z

Here is my reasoning pro keeping tests where they are and also in the .zip archives:

Some of us want the tests out, either because e.g "they pollute their autocompleter, or their code searches or just their filesystem".
Some of us want the tests in, because e.g. "having them always at hand enables this coding workflow of looking at tests to know how the code behaves and how it should be used".
You can grow the list of pro and contra arguments, the truth is that everyone is right: this is a matter of personal preference.
In this situation, we, as the Symfony project, can have two attitudes: we can forcibly decide for others, or we can empower people in allowing both way of things.
Given the history of this very topic, and the presented arguments at both sides, I'm now convinced that we should not enforce what would be otherwise a subjective and controversial "best way".
So the question for me is now: which choice allows both practices to coexist?
The first aspect is the if we always ship the tests as we do today, 2. is allowed, and 1. is just one IDE configuration or cleaning script away.
Moving tests out of each components folder breaks 2. for subtree splits. Looks like a no-go.
Now moving tests out of archives: since we install code through composer, we could use --prefer-dist for 1. and --prefer-source for 2. But this ignores one practical and fundamental difference: --prefer-source is slow as hell. --prefer-dist has one ZIP to download, and then has local caching, which means it's fast, very fast. --prefer-source has to git clone and isn't cached as of now. Nobody can't possibly accept to loose so many time because "hey, 2. is your choice, haha". So, my point is that in practice, this has the same effect as not shipping the tests at all. No-go again.

In conclusion: I'm all for the status quo, which is the best solution, unless --prefer-source can be made as fast as --prefer-dist (or any other way that would make fetching the tests as fast, see e.g. my email to github support). Meanwhile, I'm all for enhancing editorconfig or any other way to help 1. supporters configure their IDE or clean their filesystem.

Tobion · 2016-03-04T21:07:51Z

For me prefer-source is faster once you already have the git cloned.

nicolas-grekas · 2016-03-04T21:43:53Z

Here is the response from github (see above for the question): This isn't currently possible but you could create a release. This would allow you to create a ZIP file that includes the files the you want to include: https://help.github.com/articles/creating-releases/ Also please check the link to editorconfig, it's getting some attention.

mlocati · 2016-05-20T12:32:22Z

@nicolas-grekas Could you reconsider this PR? I'm totally with @rdohms comment

nicolas-grekas · 2016-05-20T14:31:34Z

@mlocati this is open source, so I invite you to advocate your ideas :)
Personally, I gave my reasoning above and I didn't get convinced yet that it needed to evolve.
This is only my humble opinion...

mlocati · 2016-05-23T08:11:22Z

Here's a soluton: composer/composer#5367

…dist" (Nyholm) This PR was merged into the 4.4 branch. Discussion ---------- Adding .gitattributes to remove Tests directory from "dist" | 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](https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/xml_with_wrong_ext.php)) ## 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 * `find vendor/symfony -name "Tests" -type d -exec rm -r "{}" \;` * editorconfig/editorconfig#228 * https://github.com/dg/composer-cleaner Commits ------- ac7dc24 Adding .gitattributes to remove Tests directory from "dist"

…dist" (Nyholm) This PR was merged into the 4.4 branch. Discussion ---------- Adding .gitattributes to remove Tests directory from "dist" | 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](https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/xml_with_wrong_ext.php)) ## 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 (symfony/symfony#29277) Added .gitattributes files to root and all components (symfony/symfony#26472) Exclude non-essential files from Composer package (symfony/symfony#25414) [HttpFoundation] optimize files for distribution (symfony/symfony#24427) Add .gitattributes files (symfony/symfony#23926) [Suggestion] Adding .gitattributes to ignore unnecessary folders and files for production env (symfony/symfony#20057) Add lightweight and root only .gitattributes (symfony/symfony#18004) Add .gitattributes to exclude tests from ZIPs (symfony/symfony#17995) [RFC] Move tests out of the source and source out of the tests (symfony/symfony#17749) Removal of development & testing files using .gitattributes (symfony/symfony#16174) Please add .gitattributes files and fix line endings (symfony/symfony#13521) making use of .gitattributes (symfony/symfony#11810) ## Workarounds There are workarounds for both sides. Example: ### Workaround if merged * `composer update --prefer-source` ### Workaround if closed * `find vendor/symfony -name "Tests" -type d -exec rm -r "{}" \;` * editorconfig/editorconfig#228 * https://github.com/dg/composer-cleaner Commits ------- ac7dc24bcb Adding .gitattributes to remove Tests directory from "dist"

Revert "removed the .gitattributes files (closes symfony#6605, reverts …

a7dfc3a

…symfony#5674)" This reverts commit b33d5bc.

carsonbot added the Status: Needs Review label Mar 3, 2016

stof reviewed Mar 3, 2016
View reviewed changes

nicolas-grekas force-pushed the gitattr branch 3 times, most recently from 257a802 to 7b77dc8 Compare March 3, 2016 10:35

javiereguiluz mentioned this pull request Mar 3, 2016

Updated all the README files #17997

Closed

fabpot reviewed Mar 3, 2016
View reviewed changes

nicolas-grekas force-pushed the gitattr branch from 7b77dc8 to b931022 Compare March 3, 2016 12:48

stof reviewed Mar 3, 2016
View reviewed changes

Add missing .gitattributes

dc4eafa

nicolas-grekas force-pushed the gitattr branch 19 times, most recently from c65d15d to 01955a5 Compare March 3, 2016 15:56

nicolas-grekas force-pushed the gitattr branch 6 times, most recently from 83cd775 to dc4eafa Compare March 3, 2016 17:04

nicolas-grekas closed this Mar 3, 2016

nicolas-grekas mentioned this pull request Mar 3, 2016

Add lightweight and root only .gitattributes #18004

Closed

nicolas-grekas deleted the gitattr branch March 4, 2016 08:11

jvasseur mentioned this pull request Mar 7, 2016

avoid Kernel::VERSION when detecting features sonata-project/SonataAdminBundle#3638

Merged

Slamdunk mentioned this pull request Mar 31, 2016

[RFC] Move tests out of the source and source out of the tests #17749

Closed

mlocati mentioned this pull request May 23, 2016

Add an option to composer.json to remove files on production composer/composer#5367

Closed

jakzal mentioned this pull request Sep 26, 2016

[Suggestion] Adding .gitattributes to ignore unnecessary folders and files for production env #20057

Closed

Nyholm mentioned this pull request Sep 14, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add .gitattributes to exclude tests from ZIPs #17995

Add .gitattributes to exclude tests from ZIPs #17995

Uh oh!

nicolas-grekas commented Mar 3, 2016

Uh oh!

stof Mar 3, 2016

Uh oh!

nicolas-grekas Mar 3, 2016

Uh oh!

fabpot Mar 3, 2016

Uh oh!

stof Mar 3, 2016

Uh oh!

nicolas-grekas commented Mar 3, 2016

Uh oh!

nicolas-grekas commented Mar 3, 2016

Uh oh!

nicolas-grekas commented Mar 4, 2016

Uh oh!

Tobion commented Mar 4, 2016

Uh oh!

nicolas-grekas commented Mar 4, 2016 via email

Uh oh!

mlocati commented May 20, 2016

Uh oh!

nicolas-grekas commented May 20, 2016

Uh oh!

mlocati commented May 23, 2016

Uh oh!

Uh oh!

Uh oh!

Add .gitattributes to exclude tests from ZIPs #17995

Add .gitattributes to exclude tests from ZIPs #17995

Uh oh!

Conversation

nicolas-grekas commented Mar 3, 2016

Uh oh!

stof Mar 3, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Mar 3, 2016

Choose a reason for hiding this comment

Uh oh!

fabpot Mar 3, 2016

Choose a reason for hiding this comment

Uh oh!

stof Mar 3, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Mar 3, 2016

Uh oh!

nicolas-grekas commented Mar 3, 2016

Uh oh!

nicolas-grekas commented Mar 4, 2016

Uh oh!

Tobion commented Mar 4, 2016

Uh oh!

nicolas-grekas commented Mar 4, 2016 via email

Uh oh!

mlocati commented May 20, 2016

Uh oh!

nicolas-grekas commented May 20, 2016

Uh oh!

mlocati commented May 23, 2016

Uh oh!

Uh oh!