Skip to content

Export-ignore test files#691

Merged
stof merged 2 commits into
minkphp:masterfrom
barryvdh:patch-1
Feb 26, 2016
Merged

Export-ignore test files#691
stof merged 2 commits into
minkphp:masterfrom
barryvdh:patch-1

Conversation

@barryvdh
Copy link
Copy Markdown
Contributor

This will exclude the tests en driver-testsuite (+ some other files) from the archives, downloaded with Composer and directly via Github. As this code is not used in production, it doesn't have to be included.

People can still use --prefer-source to get the full archive with tests etc. Should also prevent code from being run when the vendor folder is not properly protected (#677)

@barryvdh barryvdh mentioned this pull request Feb 24, 2016
@aik099
Copy link
Copy Markdown
Member

aik099 commented Feb 24, 2016

The driver repos are using Mink as dependency and are running driver-testsuite. Won't this change prevent them from working? I suppose we need to make sure --prefer-source option is used in all driver .travis.yml files.

@barryvdh
Copy link
Copy Markdown
Contributor Author

Sorry, I don't really know the internals of Mink, so perhaps there are better solutions. I would just like to suggest to package only what is needed.

If other packages are relying on the driver-testsuite for their tests, wouldn't it be better to create a seperate repository for the driver-testsuite? You can require-dev that package from here and the other drivers.

@barryvdh
Copy link
Copy Markdown
Contributor Author

But yeah, I don't know how many drivers there are, and if they're all maintained by you, but adding --prefer-source to the Travis script would be a simple solution also :)

@aik099
Copy link
Copy Markdown
Member

aik099 commented Feb 24, 2016

These are the all drivers maintained by Mink: https://github.com/minkphp?utf8=%E2%9C%93&query=Driver
This are 3rd party drivers that I'm aware of: https://github.com/jcalderonzumba/MinkPhantomJSDriver

Comment thread .gitattributes Outdated
@@ -0,0 +1,8 @@
/driver-testsuite export-ignore
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This folder is necessary when installing Mink as a dependency of a driver, as it is a testsuite meant to be used by drivers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See comment here: #691 (comment)

If other packages are relying on the driver-testsuite for their tests, wouldn't it be better to create a seperate repository for the driver-testsuite? You can require-dev that package from here and the other drivers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, the issue with this setup would be versionning for changes in Mink. But yeah, this is something I already thought about.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes and the alternative is using --prefer-source in the other drivers travis setup.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not only on Travis but for anyone working on drivers, making contributing harder. And forcing to use source installs is also bad as it means that all other deps of the driver will also be installed from source, forbidding to use the cache (making builds slower)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, the builds mostly aren't cached anyways because of API limits (they're cloning as fallback): https://travis-ci.org/minkphp/MinkGoutteDriver/jobs/98220899

But agreed, it would make it harder for them. But it seems a bit odd to include a development dependancy for everyone. As a seperate repository, it wouldn't have to be included for the rest of the users (the 99%)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, the cache contains older archives. At one point, we were lucky to avoid hitting the rate limit.

I totally agree that a separate repo would be better for this particular reason. But it is not done yet, so excluding it from the archive today is wrong.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've removed that line.

Comment thread .gitattributes
@@ -0,0 +1,7 @@
/tests export-ignore
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this folder somehow used, when running driver test suite as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is that possible, when it's only autoloaded on dev?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've double checked with one of the drivers (see https://github.com/minkphp/MinkSelenium2Driver/blob/master/phpunit.xml.dist#L7) and it seems, that only driver-testsuite folder is included in PHPUnit. So this should work theoretically.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I confirm that the driver testsuite does not need this at all.

@stof
Copy link
Copy Markdown
Member

stof commented Feb 26, 2016

OK, merging this to make the archive smaller. The case of the driver-testsuite will be handled separately

stof added a commit that referenced this pull request Feb 26, 2016
@stof stof merged commit 4b92436 into minkphp:master Feb 26, 2016
@barryvdh barryvdh deleted the patch-1 branch February 26, 2016 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants