Code review for report generation

Note: 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.

I 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?

Why 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.

Especially 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.

After 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.

Let'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.

+++ b/core/modules/migrate_drupal/tests/src/source/d6/BlockTest.php
@@ -94,13 +94,6 @@ public static function getInfo() {
-  /**
-   * {@inheritdoc}
-   */
-  public function testRetrieval() {
-    // FakeSelect does not support multiple source identifiers, can not test.
-  }
-

+++ b/core/modules/migrate_drupal/tests/src/source/d6/FieldInstanceTest.php
@@ -106,13 +106,6 @@ public static function getInfo() {
-  /**
-   * {@inheritdoc}
-   */
-  public function testRetrieval() {
-    // FakeSelect does not support multiple source identifiers, can not test.
-  }
-

+++ b/core/modules/migrate_drupal/tests/src/source/d6/NodeRevisionTest.php
@@ -342,6 +342,9 @@ protected function setUp() {
   public function testRetrieval() {
     // FakeSelect does not support multiple source identifiers, can not test.
+    // Add assertion so it passes --strict.
+    // @todo: Figure out why removing this test method results in an error.
+    $this->assertTrue(TRUE);
   }
 

Best 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?

OK 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.

Well, who do I think I am? Please do whatever you want. I am out. I said all I needed to say in #7.

@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

Couple 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.

> 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.

re: 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.

> 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.

I 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.

Updating summary to reflect discussion to date.

From #15, 1)

I've been debugging with --strict which confirms tim's comment on that issue that the timeout issue is fixed.

If this is true then I would like to mark this issue RTBC.

Because we already agreed not do strict, which has been split into three parts, one part would be useful.

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.

> 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.

> 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?

Oh, 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!

I've been debugging with --strict which confirms tim's comment on that issue that the timeout issue is fixed.
If this is true then I would like to mark this issue RTBC.

Taking 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.

Great! 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!

Yes 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:

diff --git a/core/modules/migrate_drupal/tests/src/source/d6/BlockTest.php b/core/modules/migrate_drupal/tests/src/source/d6/BlockTest.php
index 2a02af9..052abee 100644
--- a/core/modules/migrate_drupal/tests/src/source/d6/BlockTest.php
+++ b/core/modules/migrate_drupal/tests/src/source/d6/BlockTest.php
@@ -94,13 +94,6 @@ public static function getInfo() {
     );
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function testRetrieval() {
-    // FakeSelect does not support multiple source identifiers, can not test.
-  }
-
 }
 

and this passes. No need to mark it incomplete anymore.

What'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).

So 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.

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/AccessSubscriberTest.php
@@ -91,7 +91,7 @@ public function setUp() {
+   * Tests access denied throws aSymfony\Component\HttpKernel\Exception\AccessDeniedHttpException exception.

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...

I 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.

Please do not credit me. Added the suggested commit message.

It is abstract, and it does not end in 'Test'. Its test methods will not be run by PHPUnit. Got it?

This is the miscommunication. This is not actually correct. Try adding this to MigrateSqlSourceTestCase::testRetrieval()
$this->assertFalse(true);

> 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.

You 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.

Committed 045d3ed and pushed to 8.x. Thanks!