Tests for Annotation Component

Patch no longer applies cleanly.

+++ b/core/tests/Drupal/Tests/Component/Annotation/AnnotatedClassDiscoveryTest.php
@@ -0,0 +1,46 @@
+    $discovery = new AnnotatedClassDiscovery(['com/example' => [__DIR__ . '/Fixtures']]);
...
+        'class' => 'com/example\com\example\DiscoveryTest1',

That classname looks so weird, how is "com/example" a valid namespace?

Let's set it to needs work until then :)

Do you mind leaving a comment about that? It still confused me.

Kinda goes without saying... Needs a reroll. :-)

Also we need tests so we can proceed with #2631202: Doctrine no longer supports SimpleAnnotationReader, incorporate a solution into core

Re-rolled.

+++ b/core/tests/Drupal/Tests/Component/Annotation/AnnotatedClassDiscoveryTest.php
@@ -0,0 +1,89 @@
+    $result = $reflection->invoke(
+      $discovery);
+    $this->assertEquals([
+      'com/example' => [__DIR__]
+    ], $result);

Some weird formatting. If we can fit a function on a line we should.

Generally looks like it does a good job of exercising all the classes.

Fixed issue detailed in #16 and updated the rest of the patch to conform to Drupal coding standards.

Cool, thanks. RTBC now assuming the tests will pass....

1. +++ b/core/tests/Drupal/Tests/Component/Annotation/Plugin/Discovery/AnnotationBridgeDecoratorTest.php
   @@ -34,7 +34,9 @@ public function testGetDefinitions() {
   -
   
   @@ -45,12 +47,16 @@ public function get() {
   +/**
   

   nit: I think we need these blank lines

2. +++ b/core/tests/Drupal/Tests/Component/Annotation/PluginIdTest.php
   @@ -0,0 +1,55 @@
   +    // Assert plugin starts empty regardless on constructor.
   

   nit: on => of?

These can be fixed on commit

Fixed issues detailed in #19.

#20 fixes #19.

From #10
Only thing not tested now I think it the FileCache stuff.
I can't see any interdiffs since then that add that?

This component has a dependency on drupal/core-file-cache, which is incorrect in its composer.json despite #2876725: Fix trivial core component composer bugs before 8.4.0 and #2867960: Merge Component composer.json files to account for them during build

We should exercise and test the behavior of the file cache as you can see in http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Component/Annotat...

So: The test should create an AnnotatedClassDiscovery, use it to discover, check the cache for the same information we got back, change the cache, perform the same query, get back the different cached result.

Agreed? :-)

So: The test should create an AnnotatedClassDiscovery, use it to discover, check the cache for the same information we got back, change the cache, perform the same query, get back the different cached result.

That sounds like a good solution for testing the filecache.

So I see a bunch of fails locally that look like this:

1) Drupal\Tests\Component\Annotation\AnnotatedClassDiscoveryTest::testGetPluginNamespaces
Component tests should not extend a core test base class.

That's the test listener doing it's job so yay test listener.

Fixed. I had to move some FileCacheFactory management to AnnotatedClassDiscoveryTest.

Added file cache test. It's a separate test class because it has a different setUp() than AnnotatedClassDiscoveryTest, in that we want to use caching. :-)

UnitTestCase uses \Drupal and other core-related stuff. Component tests should be runnable independent of core. See: https://github.com/paul-m/drupal_component_tester

It's true that UnitTestCase might be better placed under the Core namespace.

Check it out: BrowserTestBase is under \Drupal\Tests\, too, and it doesn't even inherit from UnitTestCase. Happy to review a re-shuffling issue if you file it.

I'm not seeing any fails locally...

Oh yeah, of course.

We could set @runTestsInSeparateProcess for those classes.

Nevermind... @runTestsInSeparateProcesses not really a good idea, actually. :-)

UnitTestCase::setUp() always sets the file cache configuration and prefix, so it's in a known state before the tests run. Other than that, the only way to make sure state doesn't bleed is to run all tests that change the file cache settings in isolated processes, and not use setUp() to manage it, which is a lot more effort and takes longer. (setUp() is run before the separate process is created, so it will still change the static and possibly leak to other tests.)

I don't like the idea of the test being responsible for resetting static state at tearDown(), because the 'old' settings could just be the leaked state we don't want.

So being consistent with UnitTestCase, we set the config of the file cache explicitly in setUp() in both classes.

I couldn't find any similar issues in the file cache component.

Also: FileCacheFactory::setConfiguration() docs: Completely not useful. :-)

Also II: Statics are evil and will bring forth the ire of the Ancient Ones who will devour your soul before you can invoke the names of the Gang of Four.

:-)

Adding review credit for @dawehner

Committed as 2d25dc4 and pushed to 8.5.x.

Hmm. Never did link this issue.