Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove composer phpunit in favor of composer test #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiemowmde
Copy link
Contributor

This fixes an actual issue: Travis did not run all tests before.

@manicki
Copy link
Member

manicki commented May 14, 2018

Hmm, I don't know if this is only the matter of personally taste, but I seem to like the "composer cs" take care of all code style stuff, "composer test" run all actual tests, and "composer ci" combine all of it. But I would not like to dictate one's preference over other ones.

Will bring it up for the discussion among developers. Not going to be the highest priority tasks though.

@thiemowmde
Copy link
Contributor Author

It appears you confused something, or I did. But I have not seen ci or cs anywhere in this code base.

It might be that you became used to the outliers in some of the WMDE codebases you are working with for a while now, but these are really just that, outliers. The upstream standard is that composer test runs all tests. We are maintaining multiple hundred extensions and libraries. The people working on these realize more and more how crucial it is to stick to common standards everybody can rely on. We can see this, for example, in all the work that is put into the MediaWiki CodeSniffer rule set.

Please be transparent about this when you are going to present this to your team.

@thiemowmde thiemowmde mentioned this pull request May 24, 2018
@bekh6ex
Copy link
Contributor

bekh6ex commented May 30, 2018

I don't mind merging this patch, but I don't see any value in it as soon as Travis issue is fixed.
What does "Normalize" mean?
And what is not normal about composer test now?

@thiemowmde thiemowmde changed the title Normalize composer test shortcut Remove composer phpunit in favor of composer test May 30, 2018
@thiemowmde
Copy link
Contributor Author

Sorry, my commit message was misleading. I fixed it. With "normal" I referred to developers in the MediaWiki universe typically expecting composer test to run everything, including PHPUnit, as elaborated above.

@bekh6ex
Copy link
Contributor

bekh6ex commented May 31, 2018

composer test runs phpunit tests already:

albe@c125:~/PhpstormProjects/php-vuejs-templating$ composer test
> composer validate --no-interaction
./composer.json is valid
> phpcs -p -s
................... 19 / 19 (100%)


Time: 1.74 secs; Memory: 8Mb

> phpunit
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

..............................................................

Time: 336 ms, Memory: 6.00MB

OK (62 tests, 35 assertions)

@bekh6ex
Copy link
Contributor

bekh6ex commented May 31, 2018

The only thing this patch is doing is removing phpunit target in composer.json in which I don't see any value - only harm (I don't want to check my codestyle every time I run tests).

@thiemowmde
Copy link
Contributor Author

What harm? I'm afraid I don't get what the benefit is of typing composer phpunit instead of phpunit.

@bekh6ex
Copy link
Contributor

bekh6ex commented May 31, 2018

albe@c125:~/PhpstormProjects/php-vuejs-templating$ phpunit
The program 'phpunit' is currently not installed. You can install it by typing:
sudo apt install phpunit

@thiemowmde
Copy link
Contributor Author

The point I really want to make is that no other codebase I have ever seen in our MediaWiki universe does have a composer phpunit command. What's the point of such commands if one can not reliably use them, but have to read composer.json first? Seriously, I'm faster typing vendor/bin/phpunit – which is what I actually do when this is all I want: composer test for everything (as CI), vendor/bin/… for individual commands.

@bekh6ex
Copy link
Contributor

bekh6ex commented May 31, 2018

I'm faster typing vendor/bin/phpunit

You can continue doing it, it still works.

no other codebase I have ever seen in our MediaWiki universe does have a composer phpunit command

  1. This project is not for MediaWiki universe it is for our team in the first place
  2. I don't remember we agreed on strictly following MediaWiki universe conventions

PS: Won't reply anymore

@mariushoch
Copy link
Member

Given there has been opposition to removing the phpunit command, I don't think it should be done, as the consistency gain is negligible.

The other changes (to the README and .travis.yml) are IMO uncontroversial and desirable, though. If this PR is reduced to these changes, I'm happy to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants