Remove unused local variables from core/tests

The patch doesn't apply; it needs to be rebased.

Rerolling!

Let's also check the rest of the tests in this directory and confirm that there are no other unused local variables.

No more unused variables found in asset form me.
Regards!

I checked and there are no other unused local variables in this directory.

4: removeunused-2081153-4.patch queued for re-testing.

Applies totaly fine and there aren't any other unused local variables.

4: removeunused-2081153-4.patch queued for re-testing.

4: removeunused-2081153-4.patch queued for re-testing.

4: removeunused-2081153-4.patch queued for re-testing.

4: removeunused-2081153-4.patch queued for re-testing.

Applies totally fine. There are no more unused variables.

The previous fails were false alarms due to the outdated Tests in Views module. They are removed now.

I think this is Good To Go.

@xjm asked in #7 to check the rest of core/tests

I looked at the meta, most of the other core/tests... were closed fixed, one was marked wont fix (#2080299: Remove Unused local variable $record from /core/tests/Drupal/Tests/Core/Database/EmptyStatementTest.php).

But on inspecting core/tests, I *did* find other unused variables.

in core/tests/Drupal/Tests/Core/Plugin/Discovery/DerivativeDiscoveryDecoratorTest.php

   * @expectedException Drupal\Component\Plugin\Exception\InvalidDerivativeClassException
   */
  public function testInvalidDerivativeFetcher() {
    $definitions = array();
    // Do this with a class that doesn't implement the interface.
    $definitions['invalid_discovery'] = array(
      'id' => 'invalid_discovery',
      'derivative' => '\Drupal\system\Tests\Plugin\DerivativeTest',
    );
    $discovery_main = $this->getMock('Drupal\Component\Plugin\Discovery\DiscoveryInterface');
    $discovery_main->expects($this->any())
      ->method('getDefinitions')
      ->will($this->returnValue($definitions));

    $discovery = new DerivativeDiscoveryDecorator($discovery_main);
    $definitions = $discovery->getDefinitions();
  }

$definitions is unused. (if we take it out, $discovery would be unused)

Perhaps these last two lines are not needed at all, there are no asserts or expects after them. Are they just doing something and hoping that if an exception is fired, that the test will fail? Oh, the docs say expectedException.

also:
/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
$date in

   * @expectedException \Exception
   */
  public function testInvalidDates($input, $timezone, $format, $message) {
    $date = DateTimePlus::createFromFormat($format, $input, $timezone);
  }

Similar? does it need a var to assign a return value to, but is just checking if an exception is fired?

I tried googling a bit and found http://phpunit.de/manual/3.7/en/appendixes.annotations.html but .. not really sure.

ok. got some info from irc.

we dont need to store the result.
only need code to trigger the exception, it wont return from the call to even try and store the result.

As far as I can tell, this is now taking out all the unused vars in core/tests (that are not being handled by other issues, or aren't won't fixed in other issues)

Inspecting testbot output from #23

Drupal\image\Tests\ImageFieldDisplayTest 140 passes, fails 2

Local browser results 168 passes, 0 fails

Requesting retest,,, the number of tests was 142 now 168 HEAD has changed too much in a week

23: removeunused-2081153-23.patch queued for re-testing.

Yeah green

Visual Inspection of patch reveals nothing I have not seen before in "Remove unused local variables" like tasks.

So +1

the latest patch is not able to apply any more.working on reroll.

Rerolled patch.

Added new patch with only some of the changes from previous patch.

31: removeunsused-2081153-30.patch queued for re-testing.

I can confirm, that PHP Storm says, that these variables are unused and that they get deleted by the patch in #31.

Patch is still working.

31: removeunsused-2081153-30.patch queued for re-testing.

ok this is very strange..in #23 we have a patch that #30 is trying to reroll but that has failed. In #31 it suddenly a new patch not based on #23 and this one is then retested today. No explanations on what some of changes are added and apparently from a patch that has failed..

We need a proper reroll of #23 or a good explanation on #31

+++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/DerivativeDiscoveryDecoratorTest.php
@@ -84,7 +84,7 @@ public function testGetDerivativeFetcherWithAnnotationObjects() {
-    $definitions = $discovery->getDefinitions();

$definitions are used in the first assertEquals so can't be removed

and it would be nice if could get an interdiff so we know what's the latest patch is related too

(note, did this investigation before #38 was posted)
====
1st, what happened to the unused vars from #23 and #30:

#23 addressed
DateTimePlusTest CssCollectionRendererUnitTest DerivativeDiscoveryDecoratorTest MimeTypeMatcherTest
#30 did also.

but in #23
- $definitions = $discovery->getDefinitions();
+ $discovery->getDefinitions();
}
was in function testInvalidDerivativeFetcher()

this was fixed in #2168159: Plugin Derivatives don't work with Objects returned from Annotations

the reroll in #30
- $definitions = $discovery->getDefinitions();
+ $discovery->getDefinitions();

// Ensure that both test derivatives got added.
$this->assertEquals(2, count($definitions));

took out a used $definitions from testGetDerivativeFetcherWithAnnotationObjects()
oops!
so that should not be done.

#2019123: Use the same canonical URI paths as for HTML routes took out MimeTypeMatcherTest
(I think AcceptHeaderMatcherTest::testNoRouteFound() has the equivalent test, and it does not have the unused var. So, ok.)

but #31 is still missing the DateTime one.

--

#2080299: Remove Unused local variable $record from /core/tests/Drupal/Tests/Core/Database/EmptyStatementTest.php is a separate (won't fix) issue.

--
new unused vars:

core/tests/Drupal/Tests/Core/Menu/LocalTaskIntegrationTest.php
$request = $this->getMock('Symfony\Component\HttpFoundation\Request');
#2223615: Use request stack in local task/action manager took out where $request was used, but didn't take out where it was set.

core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
testValidateParameterTypes()
$ignored_token = $this->generator->get();
testInvalidParameterTypes()
$ignored_token = $this->generator->get();
from #2245003: Use a random seed instead of the session_id for CSRF token generation (but the interdiff there didn't show the unused vars being added.... :( )

=============
patch coming.

#38 added back taking out the DateTimePlusTest removing of the unused var. good.
Some weirdness in CssCollectionRendererUnitTest newlines after removing unused code, so fixing that.

Interdiff shows that, and also removing the new unused vars from LocalTaskIntegrationTest and CsrfTokenGeneratorTest

---
Rerunning the inspector in phpstorm for unused vars in core/tests shows only the wont fixed EmptyStatementTest one. So I think we have them all now.

---
Issue summary is up-to-date.

Probably the whole patch is the one to review.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
@@ -99,7 +99,7 @@ public function testValidate() {
+    $this->generator->get();

is this needed? A get function should always return a value and which previous line indicated, if we don't use that value the call for this function shouldn't be needed at all.

@filijonka the comment above it is:
"// Ensure that there is a valid token seed on the session."

so seems like it needs the get to ensure there is a *seed*.

Maybe that line needs a better comment?

omg how could I not see that line!? well the answer is clear and in this case the get is also used to add the value to the session.

I think that was the only thing that caught my eye so probably rtbc but will do one more check before I do that

Sit down to reroll now.

Patch still applies... looks like testbot may be down

close inspection of fail reveals

"No space left on device"

will force retest in a short while....

43: removeunused-2081153-43.patch queued for re-testing.

Ok have seen testbot behave itself in other issues

ok. green again. rtbc per #47.

Patch looks good to me

Shamelessly nominating this for a quickFix

BUT realistically recognise this may be LOW priority as it will be NOT be affected by the looming May 27 deadline
https://drupal.org/node/2247991

@alexpott said let's wait for the disruptive patch window, which would be May 29 since Alpha 12 is due May 28: https://groups.drupal.org/node/422743

Thanks for the prompt response....

The patch in #43 does not remove all unused variables.

Sorry, I dont know how I missed that one.
I checked it again, and this does remove all the unused variables.

And that iterator change looks fine.

Committed e1351af and pushed to 8.x. Thanks!