Suggested commit message:
Issue #2298667 by Mile23, neclimdul: Fixed Code review for report generation.
Problem/Motivation
I was trying to generate a PHPUnit-based coverage report for Drupal 8.
As it turns out, there are a number of --strict
violations, as well as a @covers
that specified a non-existent method.
We have previously decided to use strict but had to remove strict from our default configurations because of a but in phpunit that was fixed when we upgraded to phpunit 4. #2057905-18: [policy, no patch] Discuss the standards for phpunit based tests
The conversation about whether to enforce strict in phpunit.xml.dist is taking place here: #2105583-13: Add some sane strictness to phpunit tests to catch risky tests For the purposes of this issue, we're fixing strict errors, not enforcing strict for all of Drupal.
Proposed resolution
Fix tests to assert the code they're testing rather then just running code. In several places this is asserting null returns from functions that are not running any code. In others it is updating the mock methods to assert that the expected code is being run.
Some of the tests fail --strict because they are incomplete. Mark those as incomplete and create a followup to finish them.
Follow up: #2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#37 | 2298667-37.patch | 9.79 KB | neclimdul |
#37 | 2298667-37.interdiff.txt | 1.28 KB | neclimdul |
#30 | 2298667_30.patch | 9.57 KB | Mile23 |
#19 | 2298667-19.patch | 21.58 KB | neclimdul |
#19 | 2298667-19.interdiff.txt | 2.05 KB | neclimdul |
Comments
Comment #1
Mile23Fixes for --strict, plus a few PHPUnit standards items.
Note that the migrate and migrate_drupal ecosystem of unit tests are very strange. See the @todo.
Also, added
@requires extension gd
for the Image tests, because a nice error message is better than one you have to figure out.Comment #2
chx CreditAttribution: chx commentedNote: the goal for the source tests were to make it possible for a large number of people to write source plugins easily. We succeeded in this.
To be honest, I never understood the point of unit tests of functions without an if or a loop. Look at EntityFormBuilderTest , what's that good for , the moment you change the implementation the test breaks. So I opted for implementing an in-memory database driver so that we actually get something useful out of the tests.
Comment #3
Mile23Ideally you write unit tests because they help you write the production code. You can exercise all the dependencies in isolation and then you know your code works before you do a PR or make a patch, without wasting a few hours waiting for SimpleTest to finish.
You can also find bugs by writing unit tests for existing code. It's a form of code review. For instance: #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.
But as far as EntityFormBuilderTest, let's look at the dependencies:
$form_object = $this->entityManager->getFormObject($entity->getEntityTypeId(), $operation);
That's a chain of interfaces, and if someone changes any of those interfaces, they can run the test and know that they broke EntityFormBuilder by doing it, since it's the easiest thing in the world to spend 30 seconds finding out that you made a horrible mistake. :-) All those mocked objects essentially declare dependencies. That's why the change risk goes down the more unit coverage you have.My main criticism of EntityFormBuilderTest would be that it has the antipattern of storing some of the dependencies on the object, and some of them as variables in the method. That's left over from SimpleTest habits, and makes no sense in this context.
Meanwhile.... Within the scope of this issue... :-)
As far as those migrate tests, it was completely unclear to me that they were related to an in-memory database, after doing due dilligence to figure it out. PHPUnit --strict is unhappy with test methods that don't have assertions in them, so I deleted the ones without any code, and which were commented as saying 'There's no test to do here.' That led to a crashy-crash in one instance, which makes no sense at all.
Is there some better way to deal with the testRetrieval() methods so that they pass with
--strict
?Comment #4
chx CreditAttribution: chx commentedI have missed the policy discussion / agreement over phpunit --strict being a requirement for Drupal 8? Care to point it out for me? What are the advantages and disadvantages?
Comment #5
benjy CreditAttribution: benjy commentedWhy are we so bothered about the tests passing with --strict anyway? Looking at the docs, it doesn't look like it gives us much other than some validation when there are no assertions and some time limit stuff.
Comment #6
dawehnerEspecially once we tried to add --strict and it causes more problems than it solves, --strict seems to be a tool for the ivory tower, if you ask me.
Comment #7
chx CreditAttribution: chx commentedAfter reviewing history, I found #2105583: Add some sane strictness to phpunit tests to catch risky tests which rolled back strict once already. timplunkett noted in closing that phpunit fixed this. However, there are http://phpunit.de/manual/4.1/en/strict-mode.html now granular options. Strict itself is an absolute no go because of the issue linked (debugging timeout).
I would strongly recommend focusing on
checkForUnintentionallyCoveredCode="true"
because that is useful. The rest is not. Please clarify whether this is what I wrote about in www.drupal4hu.com/node/389 here -- that is, if a method on a mock object is called without an expectation on it then it's a fail. If this is not what happens then I very strongly recommend pursuing that sort of strictness. I looked at it and it seemed to me you need to change the templates used which would require forking phpunit into our repo. When I asked the powers that be, there was agreement that's a possibility.Comment #8
Mile23Check the issue: I wanted to make a coverage report and couldn't, because there are bad @covers annotations. This patch fixes those problems.
And over here: #2057905: [policy, no patch] Discuss the standards for phpunit based tests under 'Best Practices,' it's a good idea for PHPUnit tests to run under --strict, even if it's not specified in the configuration file.
So here's a patch. Anyone care to review it?
Comment #9
chx CreditAttribution: chx commentedLet's make this simple: I do not think adding garbage for the sake of passing --strict is a good idea. Like
$this->assertTrue(TRUE);
seriously? There are a bunch of genuine fixes here which seem to be useful, adding/fixing @group and @cover annotations, renaming that provider so it's not a test method.I am also surprised BlockTest and FieldInstanceTest doesn't blow up by the removal of testRetrieval but hey, it's all good if it doesn't.
And, see #7 for more useful things.
Comment #10
neclimdulBest I can tell these where/are emptying the only test method in the class(inherited from the parent). That means these classes do absolutely nothing other then instantiate some classes in setUp and don't test anything?
Comment #11
Mile23@neclimdul: Golly you're right. :-)
Here's a patch with the above test classes removed.
If someone wants to come along and revive them so they actually do something, that'd be great.
Comment #12
chx CreditAttribution: chx commentedOK this is not going anywhere, fast. Earlier patches proven that for eg BlockTest now is capable of testing something compared to when it was written and yet you removing this? Absolutely not.
Comment #13
chx CreditAttribution: chx commentedWell, who do I think I am? Please do whatever you want. I am out. I said all I needed to say in #7.
Comment #14
neclimdul@mile23 I'd leave the ones that passed when you let the parent test run as chx suggested.
@chx That test is broken and sitting in the code base doing nothing isn't getting it fixed but it is blocking this issue. I filled a follow up for us to add it back once it is fixed. #2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest
interdiff against #1
Comment #15
neclimdulCouple notes:
1) I've been debugging with --strict which confirms tim's comment on that issue that the timeout issue is fixed.
2) On IRC chx's clarified on of his concerns about garbage was that we where adding a bunch of assertNull's. I reviewed the code for each of them and with one exception(in the interdiff) that was in fact what the test was testing it just wasn't being asserted. The change in the interdiff was an assertion that wasn't needed because the assertions of the tests where covered by mocks.
Comment #16
Mile23Yes. Thanks for the analysis.
The one in the interdiff (
AccessSubscriberTest::testAccessSubscriberDoesNotAlterRequestIfAccessManagerGrantsAccess()
, say it three times fast) is sort of the opposite of an @expectedException. It still needs an assertion to pass --strict, however.onKernelRequestAccessCheck()
doesn't explicitly return a value, so its return value is NULL, which we can assert. This gives us two things: 1) We pass --strict, 2) If we changeonKernelRequestAccessCheck()
to return something other than NULL, the test fails and we address it.Comment #17
chx CreditAttribution: chx commented> It still needs an assertion to pass --strict
Then don't pass strict. Don't add garbage to pass a pointless gauntlet, one that has already been rolled back. Why not focus on #7 and actually help people?
Removing that one test... eh, if you have nothing better to do. I predict the issue will be lost. I would rather see an issue to fix it and the test be left as is. If that issue gets lost, nothing of value is lost.
Yes. That means --strict won't pass, on two counts. Cry me a river, please.
Comment #18
Mile23Is there some advantage to not passing --strict?
Comment #19
neclimdulre: 17
1) I don't know why you're being so hostile to this issue chx. We don't have to add garbage, we just have to have the test actually assert something rather then just running some code. That's what tests should be doing.
2) If the issue gets lost, then we're no worse off then we are now with the test being lost in our code base. We're better off because we have some assurance we don't have broken tests. Similar to how we expect our code to run in E_STRICT but for tests.
This patch changes the mocks to trigger assertions that the correct code path is passing and then document that we're testings that no assertions are thrown. Also so things remain strict and testbot can confirm these changes are doing what they claim, switch phpunit.xml.dist to strict.
Comment #20
Mile23If the issue gets lost, then no one can run coverage reports until some smart person splits off another issue dealing only with @covers.
Yes please. :-)
Probably a point of contention in general, however.
I'd like to point out that I discovered this error *because the method doesn't throw an assertion* and I did --strict -v.
I'm happy. Thanks, @neclimdul!
Comment #21
chx CreditAttribution: chx commented> I don't know why you're being so hostile to this issue chx.
Because it doesn't help anything, because it removes a test, because it doesn't focus on the useful things. Because we already agreed not do strict, which has been split into three parts, one part would be useful.
Finally: did you just RTBC your own patch???? Please reset to needs review.
Comment #22
neclimdulI don't see a reason that this still needs review. You've given some very strong reviews which I think we've addressed. Can you elaborate any changes that are holding this back?
This "other issue" keeps coming up and we need to put this to rest. We did not agree to not use --strict. We in fact agreed to use --strict and there was no rollback and all the changes where left changed. We did remove --strict from the default configs/scripts because of a bug in phpunit that is now fixed. It would be fair to see this issue as a follow up to re-enable it.
Comment #23
neclimdulUpdating summary to reflect discussion to date.
Comment #24
lokapujyaFrom #15, 1)
If this is true then I would like to mark this issue RTBC.
From my understanding, the "useless tests" part of --strict is also useful. Doesn't that prevent us from writing a test that doesn't assert anything? Currently, I don't see a good reason to use the more granular options instead of --strict.
Comment #25
chx CreditAttribution: chx commented> Can you elaborate any changes that are holding this back?
1. You are adding at least one pointless assertNull
2. You are removing a test. A broken one, but still, the proper process is postponing this issue on fixing that test.
3. All in all, you are working on something pointless and the much more useful, very closely related issues in #7 didn't even get a followup filed.
Comment #26
Mile23A real review showing which one would be helpful.
Write it, please, and it can be discussed. I don't think forking PHPUnit for a specific behavior is within the scope of this issue.
You'll see that in this case, @neclimdul has applied constraints to mocked methods that can benefit from it, which addresses your concerns to some degree:
Actually removing three tests, all of which are self-documented no-ops, so it's really removing ZERO tests. They were marked RTBC at some point, btw, which reduces my faith in the process more than a little bit.
As far as postponing this issue: I suggest you refer all the way back to comment #3, wherein I ask in good faith: "Is there some better way to deal with the testRetrieval() methods so that they pass with --strict?"
Comment #27
chx CreditAttribution: chx commented> Actually removing three tests, all of which are self-documented no-ops, so it's really removing ZERO tests.
Actually, if you look closely at what neclimdul did (thanks!) then there is only one test removed the other two got fixed meanwhile automagically (which is not entirely unexpected). There's only one test here that needs fixing. And, we wanted migrate in core because it does something useful (unlike this issue). Yes, the process was lacking because I forgot to open issues for the supressed tests.
> Is there some better way to deal with the testRetrieval() methods so that they pass with --strict?
Yes. Fix the test. Seriously, I do not have the time to figure it out how but the other two got fixed meanwhile without anyone doing anything directly so why not put a little effort in it and fix it?
Comment #28
chx CreditAttribution: chx commentedOh, nevermind, just remove it, it would take an actual effort to fix it; you'd need to add n.nid = nr.nid AND n.vid <> nr.vid support into fakeselect, that wouldn't be fun.Oh well.You can't unittest everything. This doesn't need a followup, then. I mean, TermNodeRevision has no test either. Pity :(
Can we at least get followups for the two problems in #7? Thanks!
Comment #29
lokapujyaTaking back my RTBC. Not because of the deleted tests, but because I didn't actually verify that the debugging timeout is fixed. In fact, from re-reading https://github.com/sebastianbergmann/phpunit/issues/914, using the granular options might actually be the fix.
Comment #30
Mile23Right, so there's three things here:
1) I want to run a coverage report. And even though I'd rather we enforced strict, it's not that big a deal to me. My original patch only cares about strict to the point that I could run a coverage report, and add some fixes I found as a consequence of making that happen.
2) @neclimdul wants to enforce strict for all of everything. There is some contention about whether this would ruin the debugging experience, as per #2105583: Add some sane strictness to phpunit tests to catch risky tests. We should remove the strict setting from the patch here and finish the conversation over there.
3) migrate's tests needs some love. That has spawned another issue here: #2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest In the patch here, we will mark the tests incomplete, and if that other issue finishes first, re-roll.
Also, we need a re-roll after #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)
So here it is. migrate stuff is marked incomplete. No strict in phpunit.xml.dist. Fixes @covers and --strict issues outside of migrate.
Howzat?
Comment #31
Mile23Comment #32
chx CreditAttribution: chx commentedGreat! Thanks. However, https://www.drupal.org/files/issues/2298667-19.patch has core/modules/migrate_drupal/tests/src/source/d6/BlockTest.php and core/modules/migrate_drupal/tests/src/source/d6/FieldInstanceTest.php simply removing testRetrieval. (I mentioned this in #27 as well)
I *really* like core/modules/migrate_drupal/tests/src/source/d6/NodeRevisionTest.php -- can we add a @todo with a link to the issue?
Thanks again!
Comment #33
Mile23None of the migrate files are removed or substantially altered in #30. They only do this:
Adding @todo to those three methods, and we can re-scope #2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest
Just to reiterate: This patch adds @todo comments and marks tests as incomplete.
Comment #34
chx CreditAttribution: chx commentedYes but what I have tried to say twice already: please check #19! Two of the three are not incomplete. The test method can be removed for two. The retrieval test now works:
and this passes. No need to mark it incomplete anymore.
Comment #35
Mile23@chx, you threw as literal a tantrum as possible in an issue queue when I deleted that test. Now you're saying I'm doing the wrong thing by not deleting it.
You mocked me for not fixing the code in question, and now you want me to delete it.
Moving forward, you have lost the benefit of the doubt as far as I'm concerned. You are here to troll.
Furthermore, a no-op test is not a passing test, particularly under --strict.
The patch in #33 does a reasonable job of punting the problem to whoever wants to follow up.
Get these changes in and then remove as much as you want in #2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest. Or start work now if you want and just ignore this issue.
I won't, because I don't need a repeat of this kind of behavior from yourself.
Comment #36
chx CreditAttribution: chx commentedWhat've got here is a failure to communicate. You are not understanding what I have said, probably for 10+ followups for now. You mix up two things: removing the test method actually makes these tests do something because there's a testRetrieval is in the parent which these overrides stopped from working. This is a good thing. Because we are now testing something we didn't before.
What I was against and you actually fixed that was the complete removal of a test class/file (NodeRevisionTest).
Comment #37
neclimdulSo yeah lets leave the passing tests. I guess it wasn't clear but the parent method they where overriding has a ton of exceptions and I guess at one point they didn't pass. Now they do so the easiest fix is to remove the method on the child and let the parent tests run. Making it incomplete just means we have to remove the method in the follow up. That's not going to be the nodeRevision test because that could be a pretty difficult patch alone so its just one issue to remove those two methods.
Yeah, the strict addition probably was a reach, lets leave it out and discuss in a follow up. Probably in what specific granular options we want instead. And I just assumed testbot was going to treat incomplete as a failure because phpstorm basically treats it as one. Its going to be a little annoying in phpstorm but that's ok, lets go with it. It'll get some eyes on it for sure.
I was about to RTBC but then I saw this missing space... :( Then I realized that exception is double documented in the comments because the testing annotation, so I removed it to make it fit in 80 characters. Then I realized we documented this exception in the other test for this method even though we're not testing that exception explicitly. So removed that too and now it almost fits in 80 characters as well.
TL;DNR doc cleanups in interdiff.
So yeah, lets get this RTBC and move on.
edit: my editor lied about 80 char location...
Comment #38
chx CreditAttribution: chx commentedI think it's OK now ; although I did roll one version it wasn't my code but yours :) I just failed to get the point across to get it included so I needed to do myself.
Comment #39
chx CreditAttribution: chx commentedPlease do not credit me. Added the suggested commit message.
Comment #40
Mile23If you were to start with the something like the above, instead of "I'll mark this issue Closed (Won't Fix) in a fit of pique," and linking to a 'blog post that says "We need to fork PHPUnit in order to finish this issue," then failures of communication would be less likely.
They share the same parent, which is
Drupal\migrate\Tests\MigrateSqlSourceTestCase
. It is not a test class. PHPUnit will not run it as a test. Just so we're clear on the communication here: MigrateSqlSourceTestCase NEVER runs as itself, because PHPUnit will never find it. It is abstract, and it does not end in 'Test'. Its test methods will not be run by PHPUnit. Got it?When you delete those methods, you are leaving two test classes in code with no tests to run, with only @todos telling anyone that anything is horribly, untenably wrong. This is a bad practice for obvious reasons.
In order to rectify this situation, someone should take on fixing migrate's tests. That process is (apparently) outside the scope of this issue, so they should be marked as incomplete, and not deleted.
Isn't it interesting how in the course of this bikeshedding, we have completely reversed positions? The real problem is in migrate, and it is complex enough (read: unmaintainable enough) that no one wants to take it on.
Comment #41
neclimdulThis is the miscommunication. This is not actually correct. Try adding this to MigrateSqlSourceTestCase::testRetrieval()
$this->assertFalse(true);
Comment #42
chx CreditAttribution: chx commented> When you delete those methods, you are leaving two test classes in code with no tests to run,
Huhwhat? Most of these migrate source tests do not have a test method. They handily extend the parent and run MigrateSqlSourceTestCase::testRetrieval with the right PLUGIN_CLASS, migrationConfiguration, expectedResults and databaseContents.
Comment #43
Mile23Oh, so it's a DrupalWTF.
#2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest was created at chx' insistence that whatever's up with migrate be fixed. So I suggest you make a patch there.
Note that the issue here is whether we can generate a coverage report or not, and not whether Mile23 understands the migrate tests. #33 accomplishes this. I can't RTBC code I don't understand and frankly, want to avoid.
Comment #44
neclimdulYou shouldn't need to understand it, just accept chx's review of the code as as close as we're going to get to an expert on the test. I'm going to move #37 back to RTBC based on chx being good with the migration tests and everyone being OK with the rest of the changes at this point.
Note/Reminder to commitor: Commit message in summary.
Comment #45
alexpottCommitted 045d3ed and pushed to 8.x. Thanks!