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

Allow using duration for the --order-by option as well as for the executionOrder attribute in phpunit.xml #3682

Merged
merged 3 commits into from May 8, 2019

Conversation

localheinz
Copy link
Collaborator

@localheinz localheinz commented May 5, 2019

This PR

  • allows specifying duration as value for the order-by option via CLI as well as for the executionOrder attribute in phpunit.xml

Fixes #3681.

Also see sebastianbergmann/phpunit-documentation-english#143.

@localheinz localheinz changed the title Fix: Allow ordering tests by duration via CLI Fix: Order options May 5, 2019
@localheinz localheinz force-pushed the fix/duration branch 2 times, most recently from a94016c to b4c9c01 Compare May 5, 2019 07:13
@codecov
Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #3682 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3682   +/-   ##
=========================================
  Coverage     83.08%   83.08%           
  Complexity     3716     3716           
=========================================
  Files           143      143           
  Lines          9779     9779           
=========================================
  Hits           8125     8125           
  Misses         1654     1654
Impacted Files Coverage Δ Complexity Δ
src/TextUI/Help.php 100% <ø> (ø) 24 <0> (ø) ⬇️
src/TextUI/Command.php 70.74% <100%> (ø) 207 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 544c24c...ec6fb40. Read the comment docs.

@codecov
Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #3682 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #3682      +/-   ##
===========================================
+ Coverage     83.09%   83.1%   +0.01%     
- Complexity     3717    3719       +2     
===========================================
  Files           143     143              
  Lines          9785    9791       +6     
===========================================
+ Hits           8131    8137       +6     
  Misses         1654    1654
Impacted Files Coverage Δ Complexity Δ
src/TextUI/Help.php 100% <ø> (ø) 24 <0> (ø) ⬇️
src/Util/Configuration.php 96.39% <100%> (+0.01%) 185 <0> (+1) ⬆️
src/TextUI/Command.php 71.18% <100%> (+0.14%) 209 <0> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 025f911...45d4571. Read the comment docs.

@localheinz localheinz changed the title Fix: Order options Fix: Order-by options May 5, 2019
@localheinz localheinz changed the base branch from master to 7.5 May 5, 2019 07:20
@localheinz localheinz changed the base branch from 7.5 to 8.1 May 5, 2019 07:21
@localheinz localheinz changed the base branch from 8.1 to master May 5, 2019 07:21
@localheinz localheinz changed the base branch from master to 7.5 May 5, 2019 07:22
@localheinz localheinz changed the base branch from 7.5 to master May 5, 2019 07:25
@localheinz localheinz changed the base branch from master to 7.5 May 5, 2019 07:29
@localheinz localheinz force-pushed the fix/duration branch 6 times, most recently from d948d10 to 14daebe Compare May 5, 2019 15:30
@localheinz localheinz changed the title Fix: Order-by options Fix: Allow using duration as value for order-by option via CLI May 5, 2019
@sebastianbergmann sebastianbergmann added this to the PHPUnit 8.2 milestone May 6, 2019
@sebastianbergmann sebastianbergmann added the type/enhancement A new idea that should be implemented label May 6, 2019
@sebastianbergmann
Copy link
Owner

IMO, this is not a bugfix but new functionality. Can you please make this pull request against the master branch instead? Thanks!

@sebastianbergmann sebastianbergmann changed the title Fix: Allow using duration as value for order-by option via CLI Implement --order-by=duration CLI option May 6, 2019
@localheinz localheinz changed the base branch from 7.5 to master May 6, 2019 06:15
@@ -100,7 +100,7 @@ final class Help
['arg' => '--printer <printer>', 'desc' => 'TestListener implementation to use'],
['spacer' => ''],

['arg' => '--order-by=<order>', 'desc' => 'Run tests in order: default|reverse|random|defects|no-depends'],
['arg' => '--order-by=<order>', 'desc' => 'Run tests in order: default|reverse|random|defects|no-depends|duration'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have any preferences in regard to the order

  • in which the options are listed in the help output
  • in which the options are handled in the switch-statement

?

Personally, I would probably keep them sorted like this

  • default first
  • everything else alphabetically

If you like this, I will adjust!

Copy link
Owner

Choose a reason for hiding this comment

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

I like :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice addition :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, adjusted!

@localheinz
Copy link
Collaborator Author

@sebastianbergmann

Adjusted and pointing against master!

@localheinz
Copy link
Collaborator Author

localheinz commented May 6, 2019

@sebastianbergmann @epdenouden

Just noticed that we are apparently not handling the duration value for the executionOrder attribute via phpunit.xml, would you like me to adjust this in this PR as well?

@sebastianbergmann
Copy link
Owner

Are you saying that currently neither CLI option nor configuration directive exists to control duration sorting? If so, yes please.

@localheinz
Copy link
Collaborator Author

@sebastianbergmann

Already done! 😉

@localheinz localheinz changed the title Implement --order-by=duration CLI option Allow using duration for the --order-by option as well as for the executionOrder attribute in phpunit.xml May 7, 2019
@sebastianbergmann sebastianbergmann merged commit ac71c8c into sebastianbergmann:master May 8, 2019
@sebastianbergmann
Copy link
Owner

Thanks!

@localheinz localheinz deleted the fix/duration branch May 8, 2019 07:25
@localheinz
Copy link
Collaborator Author

Thank you, @sebastianbergmann!

@localheinz
Copy link
Collaborator Author

localheinz commented May 10, 2019

@epdenouden @sebastianbergmann

I have been wondering, perhaps the reverse value should be a modifier for the existing strategies?

That is, one could sort tests by

  • default (whatever that is, the order in which tests are found on the specific file system)
  • duration
  • random

and then reverse could be a modifier to order in the opposite direction (doesn't make a lot of sense with random, but anyway).

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement --order-by=duration CLI option
3 participants