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

Make phpunit fail if a --filter does not execute any tests #2924

Merged
merged 3 commits into from Feb 18, 2024

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Feb 18, 2024

we don't know yet how to properly escape the --filter value, but we can use --fail-on-empty-test-suite to get a non-zero exit code, so we at least get a failling github action error in case our filter is bogus

found in sebastianbergmann/phpunit#4314

@staabm
Copy link
Contributor Author

staabm commented Feb 18, 2024

ohh interessting.. we had a test-case, which only runs in PHP 8.3+

if (PHP_VERSION_ID >= 80300) {
$topics[] = ['constantAccesses83'];
}

and this case did never run in CI, because level tests only execute in PHP 8.1. that was a problem even before the test-parallelizing PR landed.

this turned into an error in the first commit of this PR, because the tests-levels-matrix job returns a list built on PHP 8.3, but the levels test job is executed on PHP 8.1 and therefore was instructed to run a test, which should skip below 8.3

@@ -145,7 +145,7 @@ jobs:
uses: "shivammathur/setup-php@v2"
with:
coverage: "none"
php-version: "8.1"
php-version: "8.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we run the level tests in all php versions? or maybe lowest and highest supported version?

Copy link
Member

Choose a reason for hiding this comment

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

No it's fine to run it just once.

Copy link
Member

Choose a reason for hiding this comment

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

E.g. there's nothing that changes for levels on different PHP versions. If some rule changes behaviour based on PHP version we test in that rule's test.

@@ -155,7 +155,7 @@ jobs:
run: "composer install --no-interaction --no-progress"

- name: "Tests"
run: "php vendor/bin/phpunit ${{ matrix.test-filter }} --group levels"
run: "php vendor/bin/phpunit ${{ matrix.test-filter }} --group levels --fail-on-empty-test-suite"
Copy link
Member

Choose a reason for hiding this comment

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

I'd move it to phpunit.xml please - we always want to check this.

Also there are other interesting options that we might add:


  executionOrder="random"
  failOnWarning="true"
  failOnRisky="true"
  failOnEmptyTestSuite="true"
  beStrictAboutOutputDuringTests="true"

From https://backendtea.com/post/phpunit-beyond-basic-config/#tldr

@ondrejmirtes
Copy link
Member

Any idea why this does not execute anything? https://github.com/phpstan/phpstan-src/actions/runs/7948332751/job/21698238578 The other executions work.

@staabm
Copy link
Contributor Author

staabm commented Feb 18, 2024

Any idea why this does not execute anything?

yes, see #2924 (comment)

@ondrejmirtes
Copy link
Member

Looks like the random execution order breaks something :) Please look into tests that set environment variables, and if each test sets up env variables it needs, and also if they are properly unset in tearDown.

@@ -13,6 +13,9 @@
use function sprintf;
use const PHP_VERSION_ID;

/**
* @runTestsInSeparateProcesses
*/
class TableErrorFormatterTest extends ErrorFormatterTestCase
Copy link
Member

Choose a reason for hiding this comment

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

I just think that this test was influenced by some other test that set an env variable.

This diff is a workaround, not a solution to the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I am still in the process of finding a proper solution. the tests don't fail locally, so I push and look at CI results

@staabm
Copy link
Contributor Author

staabm commented Feb 18, 2024

I think we are good to go

@ondrejmirtes
Copy link
Member

Awesome!

@ondrejmirtes ondrejmirtes merged commit a018f1e into phpstan:1.10.x Feb 18, 2024
225 checks passed
@staabm staabm deleted the err-empty branch February 18, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants