Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up from #2057905: [policy, no patch] Discuss the standards for phpunit based tests.
Mile23 points out our unit tests do not pass when the --strict
flag is used.
Comment | File | Size | Author |
---|---|---|---|
#23 | phpunit-2096595-23.patch | 5.18 KB | tim.plunkett |
#19 | drupal8.simpletest-module.2096595-19.patch | 5.21 KB | neclimdul |
#19 | interdiff-2096595.txt | 617 bytes | neclimdul |
#18 | 2096595-18.patch | 4.61 KB | ParisLiakos |
#18 | interdiff.txt | 911 bytes | ParisLiakos |
Comments
Comment #1
tim.plunkettTwo dataProvider methods were incorrectly named.
Refactored the other two to ensure there are always assertions.
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentednice!
we should use
@expectedException PHPUnit_Framework_Error_Notice
in the docblock here
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedor no..we need the reverse..which probably does not exist;)
Comment #4
tim.plunkettThere is no "expectedNoException", which is what we need here.
Comment #5
tim.plunkettCross post :)
Comment #6
damiankloip CreditAttribution: damiankloip commentedSo we can enforce strict in the PHPUnit configuration too.. I think we should do that here too, as Tim's patch is fixing all the current shortcomings.
The rest of the changes to the tests are RTBC for me.
Comment #7
neclimdulOne more thing. Tests failed locally for me and lead me to this. http://stackoverflow.com/a/10535787
MTimeProtectedFileStorageTest sleeps for 1 sec to trigger the mtime tests so we have to allow that tests to run in the next time bracket.
Otherwise looks great and I agree with using strict 100%.
Comment #8
neclimdulwrong interdiff...
Comment #9
dawehnerIf there is an exception thrown and not catched nor expected the test will just fail as it is.
Comment #10
Mile23More PHPUnit configuration concerns here: #2056607: /core/phpunit.xml.dist only finds top-level contrib modules
Strict makes us happy.
We'll need to do a big ol' run through of coding standards anyway. :-)
Rename to something like testLoadIncludeFromNonExistantModule().
Say 'nonexistent' instead of 'foo'. :-)
You just want to test against the documented return value of loadInclude(). It should be FALSE if the file couldn't be found: http://drupalcode.org/project/drupal.git/blob/HEAD:/core/lib/Drupal/Core...
And like @dawehner said, it will automatically fail if an exception is thrown.
Comment #11
tim.plunkettYes, so then what is the assertion in the method?
That's in HEAD.
Comment #12
Mile23Also, I feel a need to write a module called foo.
Comment #13
neclimdulSeems like there's some scope creep here? Here's the interdiff of #12
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedthe assertFalse change is sweet. but lets leave the DrupalTest.php file out, or move it to another issue
Comment #15
damiankloip CreditAttribution: damiankloip commentedI think that was accidental, it's the patch from #2089787: Unit test the \Drupal class :)
Let's remove that.
Comment #16
dawehnerOn the one hand I really like to let the tests pass strict-mode.
On the other hand this though comes with a cost: it might be a little bit more annoying for people to write tests.
Comment #17
neclimdulI think the little changes we had to do here show its not that much of a cost. We where almost doing it anyway by accident. The only weird case is a weird test.
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commented#15 accidentally reversed the assertFalse change from #12
i was a bit sceptic with this change too, but i agree with #17
Comment #19
neclimdulRan the tests again before RTBC'ing and found another TimoutException. Locks waits for 1 second in one of its test. Not sure why these don't always fail but the fix is easy.
Comment #20
damiankloip CreditAttribution: damiankloip commentedYes please.
Comment #21
Mile23Re #15: Yup, my bad. Sorry. I should have made an interdiff, at which point I would have caught it.
@dahwenher: "On the other hand this though comes with a cost: it might be a little bit more annoying for people to write tests."
It's certainly annoying to try and read a test that doesn't make explicit assertions. I'd rather unit tests be specific and considered. We could get really disciplined and require @covers... :-)
Comment #22
alexpottPatch no longer applies.
Comment #23
tim.plunkettStraight reroll, just a patch context conflict.
Comment #24
alexpottCommitted 1402c66 and pushed to 8.x. Thanks!
Comment #25
dawehnerSadly this killed debugging: #2105583: Add some sane strictness to phpunit tests to catch risky tests