NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Add .gitattributes to exclude tests from ZIPs#17995

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

nicolas-grekas wants to merge2 commits intosymfony:2.3fromnicolas-grekas:gitattr

Closed

Add .gitattributes to exclude tests from ZIPs#17995

nicolas-grekas wants to merge2 commits intosymfony:2.3fromnicolas-grekas:gitattr

Conversation

Copy link

Member

nicolas-grekas commentedMar 3, 2016

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 -- seehttps://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 inTests/. 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).

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

a7dfc3a

…symfony#5674)"This reverts commitb33d5bc.

carsonbot added the Status: Needs Review label

Mar 3, 2016

stof reviewed

Mar 3, 2016

View reviewed changes

src/Symfony/Bridge/Doctrine/.gitattributes Outdated

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

Copy link

Member

stofMar 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

nicolas-grekasMar 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

added (also with a check in the phpunit wrapper)

nicolas-grekas force-pushed thegitattr branch 3 times, most recently from257a802 to7b77dc8Compare

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

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);

Copy link

Member

fabpotMar 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fetch the source from?

nicolas-grekas force-pushed thegitattr branch from7b77dc8 tob931022Compare

March 3, 2016 12:48

stof reviewed

Mar 3, 2016

View reviewed changes

phpunit Outdated


		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";

Copy link

Member

stofMar 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This should be written to stderr instead

Add missing .gitattributes

dc4eafa

nicolas-grekas force-pushed thegitattr branch 19 times, most recently fromc65d15d to01955a5Compare

March 3, 2016 15:56

nicolas-grekas force-pushed thegitattr branch 6 times, most recently from83cd775 todc4eafaCompare

March 3, 2016 17:04

Copy link

MemberAuthor

nicolas-grekas commentedMar 3, 2016

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)

Copy link

MemberAuthor

nicolas-grekas commentedMar 3, 2016

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

nicolas-grekas closed this

Mar 3, 2016

nicolas-grekas mentioned this pull request

Mar 3, 2016

Add lightweight and root only .gitattributes#18004

Closed

fabpot added a commit that referenced this pull request

Mar 4, 2016

minor#17997Updated all the README files (javiereguiluz)

8713c7e

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

Copy link

MemberAuthor

nicolas-grekas commentedMar 4, 2016

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 togit 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.

nicolas-grekas deleted the gitattr branch

March 4, 2016 08:11

Copy link

Contributor

Tobion commentedMar 4, 2016

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

Copy link

MemberAuthor

nicolas-grekas commentedMar 4, 2016 via email

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 youwant to include:https://help.github.com/articles/creating-releases/Also please check the link to editorconfig, it's getting some attention.

jvasseur mentioned this pull request

Mar 7, 2016

avoid Kernel::VERSION when detecting featuressonata-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

Copy link

Contributor

mlocati commentedMay 20, 2016

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

Copy link

MemberAuthor

nicolas-grekas commentedMay 20, 2016

@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...

Copy link

Contributor

mlocati commentedMay 23, 2016

Here's a soluton:composer/composer#5367

mlocati mentioned this pull request

May 23, 2016

Add an option to composer.json to remove files on productioncomposer/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

fabpot added a commit that referenced this pull request

Sep 16, 2019

feature#33579Adding .gitattributes to remove Tests directory from "…

5b531b6

…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 convenientIn 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)## WorkaroundsThere 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-cleanerCommits-------ac7dc24 Adding .gitattributes to remove Tests directory from "dist"

symfony-splitter pushed a commit to symfony/mailer that referenced this pull request

Sep 16, 2019