search

liuggio / fastest

Simple parallel testing execution... with some goodies for functional tests.
MIT License
475 stars 65 forks source link

Change $_ENV variable handling #115

Closed tarlepp closed 6 years ago

tarlepp commented 6 years ago

Fixes #114

DonCallisto commented 6 years ago

Umh, first of all, you should stash this into a single commit (apart PHP 7.2 that should not be into this PR as is a totally different concept.

Moreover if you rebase this to dev, you should have already this.

Second thing: has this part a test? If not, could you add some test here? That's because when we have all test matrix running we can check if everything works even with previous versions

tarlepp commented 6 years ago

@DonCallisto I runned all the tests that library currently contains without any problems - so I assumed that would be enough. Going to close this one for now, perhaps someone else have some time to make necessary changes - unfortunately currently I haven't :(

btw, we should add some contributor guidelines so that people would know what are expected with pull requests and what not.

alexislefebvre commented 6 years ago

@DonCallisto IIRC we have an option to squash commits when merging a PR.

DonCallisto commented 6 years ago

@tarlepp yes you would be right as long as we have the code covered by tests; I'm not sure this is the case, so we should at least try to think if further tests are needed or not. First thing you can try to do is to rebase this with #116 and see if all tests with that matrix are good. If they're good, then let's see together if test are needed and someone will do them (if you can't due to your time and schedule).

btw, we should add some contributor guidelines so that people would know what are expected with pull requests and what not.

I agree with this

@alexislefebvre I forgot that we can squash them, however it was only an advice just to set a "standard" for PRs but as @tarlepp said, we should write some guidelines.