Stop reporting tests without assert as risky

[vasekbrychta]

AbstractMatcherTest->testIsNullSafe() and AbstractMatcherTest->testCopesWithUnknownTypes() execute no assertion by design. Currently there are 195 tests marked as risky due to this. This change will stop reporting all these tests as risky.

[aik099]

I have a few questions:

1. why this wasn't causing any issues in the past?
2. is there any official way (except $this->assertTrue(true); approach) to mark those 2 tests so that they don't cause any harm?

[vasekbrychta]

If you look at any recent workflow run you'll see that many tests are marked as risky - it's only a warning, so it's not marking the tests as failed.

When I started writing tests for the file matchers, I wasn't able to easily find the failures of my tests, because of the overwhelming number of reported risky tests. Unfortunately, it's not only caused by the AbstractMatcherTest, but many other tests have one or more test methods without any assertion: IsArrayContainingKeyTest, AllOfTest, AnyOfTest, CombinableMatcherTest, EveryTest, IsAnythingTest, IsEqualTest, IsIdenticalTest, IsInstanceOfTest, IsNullTest, IsSameTest, IsTypeOfTest, SetTest, OrderingComparisonTest, IsEqualIgnoringCaseTest, IsEqualIgnoringWhiteSpaceTest, MatchesPatternTest, IsArrayTest, IsBooleanTest, IsCallableTest, IsDoubleTest, IsIntegerTest, IsNumericTest, IsObjectTest, IsResourceTest, IsScalarTest, IsStringTest, UtilTest, and HasXPathTest.

This change will only clean the output of tests, but won't actually solve the underlying issue in test design. Honestly, I only wanted to fix the test environment so I'll be able to proceed with writing the file matchers without unnecessary hassle.

I think the available options are:

1. rewrite all tests in a way that allows any meaningful assertion - this should be the ultimate approach I would say,
2. use the fake assertion you mentioned with a comment (or extract it to a helper method that will mark all the tests without assertion),
3. or in worst case just increment the number of assertions by $this->addToAssertionCount(1);

Personally, I consider options 2 and 3 equivalent to setting beStrictAboutTestsThatDoNotTestAnything=false, but much more harder to reverse. If you later decide to fix the design of tests without assertion it's much easier to change this flag instead of manually searching for all tests that has some artificial assert.

Fixing the test design might not be always straightforward and might require much more effort.

For example in AbstractMatcherTest->testIsNullSafe() and AbstractMatcherTest->testCopesWithUnknownTypes() you can test for the actual output from the matches(..) and describeMismatch(...) methods - you'll make explicit what the behavior should be in this case.

[aik099]

Meaning of testIsNullSafe and testCopesWithUnknownTypes doesn't expect any assertions to happen and adding a fake one won't increase clarity. For them I'd recommend adding $this->addToAssertionCount(1); to the end.

Turned out there are 97 more tests (not counting testIsNullSafe and testCopesWithUnknownTypes), that call global Hamcrest-provided assertThat function, that however doesn't report back to PHPUnit, that assertions were actually made.

That was solved in the Mockery project via a test listener (used to work like this before, but not sure this works on recent PHPUnit version), that added performed assertion account via $this->addToAssertionCount(...); method (see https://github.com/mockery/mockery/blob/d1339f64479af1bee0e82a0413813fe5345a54ea/tests/Mockery/Adapter/Phpunit/TestListenerTest.php).

Theoretically:

• at test case setUp method the \Hamcrest\MatcherAssert::resetCount needs to be called
• at test case tearDown method the \Hamcrest\MatcherAssert::getCount method needs to be called in combination with $this->addToAssertionCount(...);

Not sure if you can do that via PHPUnit hook or a trait is needed like it's done in Mockery these days.

[pscheit]

If you have a test that does not perform assertions, then annotate the test with:

https://docs.phpunit.de/en/9.6/annotations.html#doesnotperformassertions

that being said I think adding the option to phpunit.xml.dist would be a good start - because now as a first time contributor this was confusing :)

phpvfscomposer:///home/psc/projects/hamcrest-php/vendor/phpunit/phpunit/phpunit:106

190) Hamcrest\Xml\HasXPathTest::testProvidesConvenientShortcutForHasXPathEqualTo
This test did not perform any assertions

/home/psc/projects/hamcrest-php/tests/Hamcrest/Xml/HasXPathTest.php:145

phpvfscomposer:///home/psc/projects/hamcrest-php/vendor/phpunit/phpunit/phpunit:106

191) Hamcrest\Xml\HasXPathTest::testProvidesConvenientShortcutForHasXPathCountEqualTo
This test did not perform any assertions

/home/psc/projects/hamcrest-php/tests/Hamcrest/Xml/HasXPathTest.php:151

phpvfscomposer:///home/psc/projects/hamcrest-php/vendor/phpunit/phpunit/phpunit:106

192) Hamcrest\Xml\HasXPathTest::testMatchesAcceptsXmlString
This test did not perform any assertions

/home/psc/projects/hamcrest-php/tests/Hamcrest/Xml/HasXPathTest.php:156

phpvfscomposer:///home/psc/projects/hamcrest-php/vendor/phpunit/phpunit/phpunit:106

193) Hamcrest\Xml\HasXPathTest::testMatchesAcceptsHtmlString
This test did not perform any assertions

/home/psc/projects/hamcrest-php/tests/Hamcrest/Xml/HasXPathTest.php:161

phpvfscomposer:///home/psc/projects/hamcrest-php/vendor/phpunit/phpunit/phpunit:106

194) Hamcrest\Xml\HasXPathTest::testIsNullSafe
This test did not perform any assertions

/home/psc/projects/hamcrest-php/tests/Hamcrest/AbstractMatcherTest.php:49

phpvfscomposer:///home/psc/projects/hamcrest-php/vendor/phpunit/phpunit/phpunit:106

195) Hamcrest\Xml\HasXPathTest::testCopesWithUnknownTypes
This test did not perform any assertions

/home/psc/projects/hamcrest-php/tests/Hamcrest/AbstractMatcherTest.php:59

phpvfscomposer:///home/psc/projects/hamcrest-php/vendor/phpunit/phpunit/phpunit:106

OK, but incomplete, skipped, or risky tests!
Tests: 382, Assertions: 369, Risky: 195.

[pscheit]

By the way: https://github.com/rowanhill/wiremock-php/blob/master/test/WireMock/HamcrestTestCase.php

Not ideal, but works I think. Pretty sure there is an official way to register assertion counts as well more easily in newer PHPUnit versions - 9 is now really old.

[aik099]

Thank you @pscheit for bringing my attention back to this PR. I've created #89 PR based on the both #72 (comment) comment (mine) and #72 (comment) comment (yours).

Please see if that is what you've meant.