Problem/Motivation
It's standard practice in PHPUnit tests to mark tests that don't make an assertion as risky. This is because without making any assertions you are not really testing any assumptions beyond any code being executed is not triggering an error.
However at the moment the \Drupal\KernelTests\KernelTestBase ensures that there is always one assertion. Because it make one of every tests behalf.
Proposed resolution
Remove the assertion so that issues like #2942529: Text Editor module assumes that a fieldable entity is also revisionable aren't tempted to add a test with no assertions.
With this fix running the test in #2942529-23: Text Editor module assumes that a fieldable entity is also revisionable results in the following output.
There was 1 risky test:
1) Drupal\Tests\editor\Kernel\EditorEntityTest::testEditorFunctions
This test did not perform any assertions
OK, but incomplete, skipped, or risky tests!
Tests: 1, Assertions: 0, Risky: 1.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
Previously \Drupal\KernelTests\KernelTestBase always made one assertion which would hide tests that did not preform any assertions themselves. KernelTestBase will now no longer perform one assertion by default which means that kernel tests that don't perform any assertions will now be marked as risky and will fail on Drupal CI.
Comment | File | Size | Author |
---|---|---|---|
#39 | 3059332-39.patch | 11.6 KB | mondrake |
#39 | interdiff_38-39.txt | 817 bytes | mondrake |
#38 | interdiff_36-38.txt | 2.75 KB | mondrake |
#38 | 3059332-38.patch | 11.59 KB | mondrake |
#32 | 3059332-32.patch | 16.32 KB | Lendude |
Comments
Comment #2
alexpottI've tried to cause this to occur but even reregistering shutdown functions in shutdown functions doesn't cause the assertion to fail. The only other way this could fail is if we introduce a shutdown function into the kernel shutdown method. But that really does not make sense - here's the current implementation.
So I've decided to remove the code entirely and add a test that KTB does call shutdown functions. And that shutdown functions registered in shutdown functions are called as expected.
Comment #3
alexpottIf we want we can replace the original assertion with something like
But I couldn't work out how to test this or cause this - see #2.
Comment #5
alexpottWell that's revealing.
Some stuff will still fail... working through it.
Comment #7
Mile23Adding this related issue while I'm thinking about it... #2905007: Allow run-tests.sh to report skipped/risky/incomplete PHPUnit-based tests
Comment #8
alexpottSome more fixes. I guess I'm going to break out some of these into their own issue.
I think this issue is probably critical as we think we have test coverage of things we don't currently have test coverage of.
Comment #9
Mile23I can't quite suss why we care about the number of error handlers, because in
assertPostCondition()
we already executed all of them. Do we think that maybe calling$this->container->get('kernel')->shutdown()
added some when it shouldn't?Also, assuming we want to leave this all in place, the way to do it without adding an assertion is:
Also it's weird that in assertPostCondition() we call
$this->container->get('kernel')->shutdown()
, and then in tearDown() we call:...particularly when there's not a
$this->kernel
.Comment #11
alexpottSo the deprecation fails are interesting. These are tricky because the test listener is supposed to take care of this. Doesn't work though because of test isolation. There's not really a simple way to change this or fix this as far as I can see. I will raise an upstream issue about this.
re #9 it's not error handlers it's shutdown functions and what we're trying to achieve is that all shutdown functions have been run before the test is complete and therefore are run as part of the test. However as pointed out by #2 its not really possible that extra shutdown functions are registered.
Comment #13
alexpottWhoops. Yay for deprecation testing!
Comment #14
Mile23"Yay for deprecation testing!" Famous last words. :-)
pass()
comes fromAssertLegacyTrait
, which isn't such a huge sin.I'd suggest
$this->assertNotEmpty($plugin_manager->getDefinition($display_options['type']), 'Plugin found...');
since the fail state here is an empty array fromgetDefinition()
.Needs an issue.
Needs an issue to remove, maybe? It comes from here: #2263287: Test the CachedStorage class using ConfigStorageTestBase
Needs an issue to remove? Comes from here: #2740983: Configuration system doesn't allow importing a single item from a non-default collection Copypasta from the previous one or vice-versa?
If it's me, this test gets its own test class so its readable, but that's a style nit type thing.
Comment #15
LendudeHard to review if all the changes are actually needed with everything in one big patch, but splitting this up into tiny bits doesn't seem very useful either ¯\_(ツ)_/¯. Some things I see:
Removing FilterSettingsTest completely seems a little excessive, the foreach both need to be
Which will then actually test what it should I believe
add a period at the end or maybe just leave the comment out.
#14.3 and #14.4, not sure we can remove those since they override test cases in the parent class, so this is their way of disabling those cases for these specific scenarios. I do believe the message can be updated to reflect this better, something like 'No-op as this test does not make sense for this setup.', or something along those line. Or maybe add a comment to clear this up.
Comment #16
alexpottI'm going to carve 2 things from this into other issues. To keep the scope a bit easier.
Thanks for the reviews @Mile23 and @Lendude
Re #14
Re #15
See #3059543: Remove \Drupal\Tests\filter\Kernel\FilterSettingsTest it tests nothing
Added the fullstop in UnpublishByKeywordActionTest
Note the patch attached still includes the blocking issues so we can see that we've completed the entire task.
Comment #18
alexpottComment #19
LendudeQuestion: how can we do this without causing (major) disruption in contrib? Looking at the number of fails in core, I think it is safe to say that committing this is going to break stuff in contrib tests.
Comment #20
Mile23PHPUnit CLI (6.5+) can say
--dont-report-useless-tests
.So we could leave phpunit.xml.dist as it is, and then rig up run-tests.sh to have a flag for whether to say
--dont-report-useless-tests
on PHPUnit CLI. DrupalCI could force this flag for contrib much the way it does now for deprecation errors.Or we could just enforce it here in 8.8.x with no backport to 8.7.x, and send up flares so people could try out their tests.
Comment #21
alexpottYes but, much like core this is going to break in a good way. It will remove the sense of false confidence the current test is giving the author. We can add a change record and and release note mention so people are forewarned and only put this into 8.8.x.
Comment #22
Mile23Or, you know, backport the improved tests without changing 8.7.x's KernelTestBase.
Comment #23
Mile23Issues from #16 are in.
Comment #24
borisson_I think this solution makes most sense. Adding the test improvements to reduce how much the two versions diverge.
I also think that it might be a good idea to ask/notify in #maintainers on slack so that people know this is coming up.
Comment #25
alexpottRerolled.
Comment #26
Mile23Harkening back to #14, I can't find any test that directly covers views counters with pagers, so that's why I mentioned this needs an issue. We clearly meant to add that test, and
Counter::getValue()
overrides the parent method.Hmm.... Research says this @todo started life in 2013. :-) #1872876: Turn role permission assignments into configuration.
Please add a @todo in the class docblock for #3063179: Add test coverage for Drupal\views\Plugin\views\field\Counter::getValue() that I just filed.
The rest LGTM.
Still needs a CR.
Comment #27
LendudeCR added: https://www.drupal.org/node/3063749
Comment #28
Mile23Adding @todo per #26. This is a docblock-only change.
Comment #29
LendudeAll blockers are in, follow-ups are filed, CR added, I think this is ready. Added release note snippet to the IS.
Comment #30
catchI think this is fine to commit to 8.8.x - it might result in some contrib tests failing but in a way that's easy to fix.
For 8.7.x agreed with backporting the individual test improvements only.
Committed 6abefc5 and pushed to 8.8.x. Thanks!
Comment #32
LendudeHere is a 8.7.x version
without these changes
Comment #33
mondrakePostgres https://www.drupal.org/pift-ci-job/1334205 and SQLite https://www.drupal.org/pift-ci-job/1334206 test runs now have failures on risky tests, is this the culprit?
Comment #34
mondrakeWorking on a fix
Comment #35
Mile23Both tests use
if()
to make sure the tests are for MySQL. So each of those need anelse
that saysmarkTestAsSkipped()
.Comment #36
mondrakeLet's try and do sth more sophisticated, since these tests are only valid for MySQL. This patch is introducing a namespace for driver-specific tests, and moving the two tests to the 'mysql' driver tests namespace. IMHO this is how it should be in OOP world...
Done some more cleanup of old simpletest methods like
::verbose
and::assertEqual
.Comment #37
Mile23We really should say
if ( skip_condition ) { skip }
at the top, so it's easy to read.The rest is +1. :-)
Comment #38
mondrakeDone #36.
Comment #39
mondrakeMissed 'OrEqual'...
Comment #40
Mile23Thanks.
LGTM.
Comment #41
alexpottCommitted 0dd7576 and pushed to 8.8.x. Thanks!
I'm not a huge fan of this. It's unnecessary class hierarchy making people think. And we have to remember to use it in future and check if anything should be using it now. But fixing the test fails is more important. I might file an issue to refactor this.
Removed unused use on commit.
Comment #43
alexpottCreated #3065101: Remove MySqlDriverTestBase - another reason why doing that is correct is that test requirements shouldn't dictate class structure so much - it means that if we work out or need to do the same test for another DB driver - say MariaDB if we split them or can extend this coverage to SQLite or PostgreSQL then we don't need to change everything.
Comment #45
Gábor HojtsyAs this would change behaviour of extending contrib tests, adding release notes tag.
@catch wrote above:
Comment #46
Gábor Hojtsy