Skip to content

Conversation

@dakujem
Copy link
Contributor

@dakujem dakujem commented Sep 7, 2018

Changes:

  • Updated Tester to version v2 to enable for -C switch (see below)
  • Added section to readme about testing
  • Added 2 Composer commands for testing:
    • composer test (run all tests)
    • composer test-local (run all tests using local PHP configuration, via the -C Tester switch)

Maybe some info about contribution can be added by you.

Also, what about the following sentence ??

"This library is no longer actively developed." ...

@dakujem
Copy link
Contributor Author

dakujem commented Sep 7, 2018

Oh... okay. PHP 5.4 and 5.5 issue.

@janpecha
Copy link
Collaborator

janpecha commented Sep 7, 2018

I plan to drop PHP 5 support and update Tester in Lean Mapper 4.x.

For now IMHO would be fine add only contributing.md with instructions - how to create branch for PR, how to test changes (with list of required extensions), etc. Something like https://nette.org/en/contributing.

Composer commands

IMHO it's unnecessary because everybody needs something else - set another php.ini, run to coverage, Windows vs Unix issues, etc.

"This library is no longer actively developed." ...

I think we can remove it.

@dakujem
Copy link
Contributor Author

dakujem commented Sep 7, 2018

That composer command test-local is very flexible and will work for most local environments (Win, Linux and Mac, provided they have composer installed) - all those extensions are pretty much standard for any local development. It is also super easy and even the lazy folks can run it. I personally prefer that to looking for a way to run the tests, then failing 5 times because of misspelled pathname, studying how nette Tester in particular is run (for newcommers) or similar issues.

Personally, I would only keep the local command as default for composer test, but I included that other one because I thought you were using the unix config.

@janpecha
Copy link
Collaborator

janpecha commented Sep 9, 2018

I thought about it and I have some notes.

  • vendor/bin/tester -C doesn't work in Tester 1.7 so test-local is useless for now
  • I don't want to have hardcoded vendor/bin/tester -c php-unix.ini in composer.json because in Tester 1.7 we need flexibility:
    • we need specify right php.ini for current environment
    • sometimes we need change PHP binary (-p php, -p php5.6 or something else)
    • a lot of another posibilities (watch mode,...)
  • newcomers and lazy people must always read manual and make minimal 3 steps:
    1. install required extensions (or check if are installed)
    2. run composer install
    3. run Tester - there are 2 options: composer test or vendor/bin/tester <some arguments>
  • composer test isn't flexibile in Tester 1.7 and is useless in Tester 2.x because there aren't big diferrences between composer test and simple vendor/bin/tester -C
  • good contributing.md is better than one command line shortcut

So I think contributing manual is good idea but we don't need composer test.

@dakujem
Copy link
Contributor Author

dakujem commented Sep 9, 2018

I agree with Tester v1.7 it does not bring the utility I had in mind and with upgrade to Tester v2 you lose support for legacy PHP. Though I think it is a good idea to just have composer test once you decide to move forward. It's just so easy to type composer test instead of path/to/my/vendor/some-binary some-options-why-uppercase-ffs.

@janpecha
Copy link
Collaborator

It would be great split this PR on 2 parts:

  1. manual for contributors
  2. changes in composer.json

We can add contributors manual in separate PR and this PR can wait for LM 4.x. What do you thing?

@dakujem
Copy link
Contributor Author

dakujem commented Sep 14, 2018

@janpecha Good idea. How can i do that? Or will you?

@janpecha janpecha mentioned this pull request Sep 14, 2018
@janpecha
Copy link
Collaborator

@dakujem I created #136. It's ok for you?

@janpecha janpecha added this to the Version 4.0.0 milestone Sep 14, 2018
@dakujem
Copy link
Contributor Author

dakujem commented Sep 14, 2018

I like it. Let's keep this one open until v4 (or close it, it's up to you, I don't mind).

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.

2 participants