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

Support for multiple arguments #5462

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

joke2k
Copy link
Contributor

@joke2k joke2k commented Aug 5, 2023

This work adds the support for providing multiple files or directories as arguments to PHPUnit:

./phpunit FooTest.php test/

It is very useful when you want to test multiple things together without having a Suite defined in the configuration, and most important (for me) is that it works for CircleCI's parallelisation command (apparently phpunit is one of the few test frameworks without this feature).

It could be easily extended for phpunit9.

Point to discuss

  • naming the Test suite: a proposal is using a hash (md5) of all provided arguments (the list could be very long)

@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/test-runner CLI test runner labels Aug 6, 2023
@sebastianbergmann
Copy link
Owner

It could be easily extended for phpunit9.

PHPUnit 9 is closed for changes other than bug fixes and adaptions to new versions of PHP.

@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #5462 (7305a82) into main (d0103c0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 7305a82 differs from pull request most recent head ba75b2a. Consider uploading reports for the commit ba75b2a to get more accurate results

@@             Coverage Diff              @@
##               main    #5462      +/-   ##
============================================
+ Coverage     87.06%   87.07%   +0.01%     
- Complexity     6205     6211       +6     
============================================
  Files           662      662              
  Lines         19858    19874      +16     
============================================
+ Hits          17290    17306      +16     
  Misses         2568     2568              
Files Changed Coverage Δ
src/TextUI/Configuration/TestSuiteBuilder.php 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@joke2k
Copy link
Contributor Author

joke2k commented Aug 6, 2023

Thank you for the reply and to take into consideration this work.

I applied the CSFixer rules, sorry about that.

I was wondering how to tell to psalm that $configuration->cliArgument() cannot raise a NoCliArgumentException in the TestSuiteBuilder::build because a few lines above $configuration->hasCliArgument() has been checked.

I tried several solutions to avoid that MissingThrowsDocblock, but I'm not able to fix it yet even if I add @throws NoCliArgumentException to the method, which is not true.
Seems that psalm forgets the @psalm-assert-if-true !null $this->cliArgument on the Configuration::hasCliArgument.

I don't understand why this issue didn't appear before my changes

@joke2k
Copy link
Contributor Author

joke2k commented Aug 7, 2023

@sebastianbergmann do you have any ideas on how to satisfy psalm in this case?

@sebastianbergmann
Copy link
Owner

I do not have time to look into this right now.

@joke2k
Copy link
Contributor Author

joke2k commented Aug 8, 2023

@sebastianbergmann this is ready to be checked again. psalm issue should be fixed.

Thank you

$argument,
$configuration->testSuffixes(),
);
$testSuite = count($arguments) === 1

Choose a reason for hiding this comment

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

Please do not use the ternary operator here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

splitted using a more readable if-else.

@sebastianbergmann
Copy link
Owner

naming the Test suite: a proposal is using a hash (md5) of all provided arguments (the list could be very long)

As of 51f93ba, the name of the top-level test suite that is created when a directory or file path is passed as an argument to the test runner is now always CLI Arguments.

@sebastianbergmann sebastianbergmann added this to the PHPUnit 10.4 milestone Aug 21, 2023
sebastianbergmann added a commit that referenced this pull request Aug 23, 2023
@sebastianbergmann
Copy link
Owner

Thank you for patience. I did not like the implode() ... explode() workaround that you had to use. As of d0103c0, this is no longer necessary. Once you adapted your PR to that, I think we can merge this. Thanks!

@joke2k
Copy link
Contributor Author

joke2k commented Aug 23, 2023

Adapted the code with your main to use the new cliArguments methods.
I tested locally but I recently updated my php to 8.1.22 and I receive some wired error on some phpt tests like:

/open-source/phpunit/tests/end-to-end/regression/765.phpt
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 ERRORS!
 Tests: 1, Assertions: 1, Errors: 1.
+Standard input code:8:
 int(2)

but I can see that your pipeline is working on the main, so I think is an issue only on my laptop.

@sebastianbergmann
Copy link
Owner

This branch cannot be rebased due to conflicts.

@joke2k
Copy link
Contributor Author

joke2k commented Aug 24, 2023

Hi @sebastianbergmann, I squashed all the commits and rebased the branch.

@sebastianbergmann sebastianbergmann merged commit de1f767 into sebastianbergmann:main Aug 25, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-runner CLI test runner type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants