Clean-up Test members in core/tests - ensure property definition and use of camelCase naming convention

A couple of properties of CssOptimizerUnitTest were unused. Other than that, very straightforward.

Also:

$ phpcs --standard=".../drupal/coder/coder_sniffer/Drupal/" --report-csv core/tests/ | grep "should use lowerCamel"
$

Patch looks good to me. Issue needs beta evaluation though?

I think we missed one.

Thanks! As you can tell, I haven't run a grep or sniffer to see that there are not more problems, but everything in this latest patch still looks great.

@cilefen hehe sorry about that, i am a native spanish and my english isn't so good :P

#11.1: Done.

#11.2: Yup. It happens. :-)

#11.3: TestThemeHandler is really a stub class, and should be renamed. I'm not sure how the interaction works with its superclass, so I'll just leave it.

#11.4: No, what would be better is not having that test dependency as a class property. :-)

#11.5: Done.

Couple of nitpicks, but this one looks really close.

1. +++ b/core/tests/Drupal/Tests/Component/Plugin/Discovery/DiscoveryCachedTraitTest.php
   @@ -16,10 +16,28 @@
   +  protected $definitionsRef;
   

   Any chance we can rename this to $definitionReflection? I thought it was a Reference at first glance.

2. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
   @@ -84,7 +91,7 @@ public function providerEscapeFields() {
   +    $pgsql_connection = new Connection($this->mockPdo, array());
   
   @@ -94,7 +101,7 @@ public function testEscapeTable($expected, $name) {
   +    $pgsql_connection = new Connection($this->mockPdo, array());
   
   @@ -104,7 +111,7 @@ public function testEscapeAlias($expected, $name) {
   +    $pgsql_connection = new Connection($this->mockPdo, array());
   

   Might as well use short array syntax for these as they don't change the diff hunk.

Done.

+++ b/core/scripts/run-tests.sh
@@ -949,7 +949,7 @@ function simpletest_script_reporter_write_xml_results() {
-          file_put_contents($args['xml'] . '/' . str_replace('\\', '_', $test_class) . '.xml', $xml_files[$test_class]['doc']->saveXML());
+          file_put_contents($args['xml'] . '/' . $test_class . '.xml', $xml_files[$test_class]['doc']->saveXML());

@@ -1003,7 +1003,7 @@ function simpletest_script_reporter_write_xml_results() {
-    file_put_contents($args['xml'] . '/' . str_replace('\\', '_', $test_class) . '.xml', $xml_files[$test_class]['doc']->saveXML());
+    file_put_contents($args['xml'] . '/' . $test_class . '.xml', $xml_files[$test_class]['doc']->saveXML());

I think some things may have snuck in from another patch or did you mean to do this here?

You should git pull origin 8.0.x. :-)

Ok maybe my question wasn't all that clear:

#12 didn't have that changed hunk in it. #14 has that hunk at the top but the comment was just 'Done' in #14. So I'm curious if you meant to do that change on purpose or did it accidentally come from another patch or maybe forgot a rebase?

If it was on purpose, please let us know why it was needed.

Ahh maybe I need a rebase. Thanks.

Trying again... Interdiff is from #12, not #14.

Thanks @Mile23. I see you renamed $definitions_ref to $ref_definitions. What were ideas on that change?

It kinda reads better to me but why not also $reflection_definitions?

Thanks for reviewing these.

Here's another approach: Let's just use an anonymous function. If it's out of scope just ignore this patch and review #19. :-)

The $ref_ prefix is easy to read and in wide use in D8 and the rest of the world to mean 'reflection object for _name'.

We tend way from short variable names, like $variables instead of $vars. Though no hard rule that I could find on it. And $ref as I mentioned before is likely more common as "reference" and not "reflection".

#21 Is ok with me as it's a nice cleanup (more compact way to deliver the test, nice work!) but a couple of notes on coding standards:

1. +++ b/core/tests/Drupal/Tests/Component/Plugin/Discovery/DiscoveryCachedTraitTest.php
   @@ -72,13 +48,16 @@ public function testGetDefinition($expected, $cached_definitions, $get_definitio
   +        ->willReturnCallback(function() use ($ref_definitions, $trait, $get_definitions ) {
   

   whitespace before the last ) should be removed.

2. +++ b/core/tests/Drupal/Tests/Component/Plugin/Discovery/DiscoveryCachedTraitTest.php
   @@ -72,13 +48,16 @@ public function testGetDefinition($expected, $cached_definitions, $get_definitio
   +          $ref_definitions->setValue(
   +            $trait, $get_definitions
   +          );
   

   This should be one line without the whitespace around the arguments.

And $ref as I mentioned before is likely more common as "reference" and not "reflection".

In the context of a unit test, not so much. But no biggie.

Got the formatting stuff.

Beauty, thanks, this looks like a great cleanup.

+++ b/core/tests/Drupal/Tests/Core/Routing/RouteBuilderTest.php
@@ -51,7 +51,7 @@ class RouteBuilderTest extends UnitTestCase {
-   * The mocked yaml discovery.
+   * The mocked YAML discovery.

@@ -281,6 +281,13 @@ public function testRebuildIfNeeded() {
+   * The mocked yaml discovery.

To capitalize or not to capitalise?

+++ b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
@@ -17,13 +17,6 @@
-  protected function setUp() {
-    $this->parser = new LogMessageParser();
-  }

@@ -39,7 +32,8 @@ protected function setUp() {
-    $message_placeholders = $this->parser->parseMessagePlaceholders($value['message'], $value['context']);
+    $parser = new LogMessageParser();
+    $message_placeholders = $parser->parseMessagePlaceholders($value['message'], $value['context']);

This change subtly changes the nature of the test - before we were also testing that repeated calls to the same log parser had no effect on each other. Is this a desired change?

This change subtly changes the nature of the test - before we were also testing that repeated calls to the same log parser had no effect on each other. Is this a desired change?

setUp() is called before each test method. So a) It wasn't the same parser anyway, assuming more than one test method, and b) there's only one test method so it doesn't matter. Setting test dependencies as class properties is an anti-pattern you see all through D8, inherited from SimpleTest.

Also: YAML CAPITALIZATION MODE: ACHIEVED!

I believe the issues were addressed in #26 resetting to RTBC.

#25.1 If it is a desired test multiple calls to the same logger, it may be pertinent to make it explicit test case. Though I'd push for that to go in another issue.

25.2 Consistency is good, good catch @alexpott and uppercase is a good choice since it's an acronym.

Committed 523b60b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.