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 May 5, 2019
@codecov
Copy link

@codecov 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.

Loading

@codecov
Copy link

@codecov 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.

Loading

@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
@localheinz localheinz changed the base branch from 7.5 to 8.1 May 5, 2019
@localheinz localheinz changed the base branch from 8.1 to master May 5, 2019
@localheinz localheinz changed the base branch from master to 7.5 May 5, 2019
@localheinz localheinz changed the base branch from 7.5 to master May 5, 2019
@localheinz localheinz changed the base branch from master to 7.5 May 5, 2019
@localheinz localheinz force-pushed the fix/duration branch 6 times, most recently from d948d10 to 14daebe May 5, 2019
@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
Copy link
Owner

@sebastianbergmann sebastianbergmann commented May 6, 2019

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

Loading

@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
@@ -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

@localheinz localheinz May 6, 2019

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!

Loading

Copy link
Owner

@sebastianbergmann sebastianbergmann May 6, 2019

Choose a reason for hiding this comment

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

I like :)

Loading

Copy link
Collaborator

@epdenouden epdenouden May 6, 2019

Choose a reason for hiding this comment

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

Nice addition :)

Loading

Copy link
Collaborator Author

@localheinz localheinz May 6, 2019

Choose a reason for hiding this comment

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

Thanks, adjusted!

Loading

@localheinz
Copy link
Collaborator Author

@localheinz localheinz commented May 6, 2019

@sebastianbergmann

Adjusted and pointing against master!

Loading

@localheinz
Copy link
Collaborator Author

@localheinz 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?

Loading

@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented May 6, 2019

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

Loading

@localheinz
Copy link
Collaborator Author

@localheinz localheinz commented May 6, 2019

@sebastianbergmann

Already done! 😉

Loading

@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
4 checks passed
Loading
@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented May 8, 2019

Thanks!

Loading

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

@localheinz localheinz commented May 8, 2019

Thank you, @sebastianbergmann!

Loading

@localheinz
Copy link
Collaborator Author

@localheinz 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?

Loading

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

Successfully merging this pull request may close these issues.

3 participants