Expand unit testing for Drupal\Core\Plugin\Context\ContextDefinition

Since these are testing, as you say, the two most complex methods, I'd like to see a few code comments just to clarify the flow of these tests. It wasn't immediately obvious to me exactly where or when the exception in testGetDataDefinitionInvalidType() was going to be thrown.

Those comments look good to me.

Does it make sense to visit \Drupal\system\Tests\Plugin\ContextPluginTest and remove some (all?) of those tests as part of this?

I'd say the more tests the better, and the less we muck about with other tests the better.

I don't fully agree with that--some of the tests in the ContextPluginTest are identical (fake unit tests). At least those should be removed to avoid unnecessary effort in the future.

While ContextPluginTest is using functional items for testing (users, nodes, and blocks), most of that test still reads to me like a unit test. For instance, isn't this

   // Try to get context that is missing its definition.
    try {
      $plugin->getContextDefinition('not_exists');
      $this->fail('The user context should not yet be set.');
    }
    catch (ContextException $e) {
      $this->assertEqual($e->getMessage(), 'The not_exists context is not a valid context.');
    }

or

  // Try to pass the wrong class type as a context value.
    $plugin->setContextValue('user', $node);
    $violations = $plugin->validateContexts();
    $this->assertTrue(!empty($violations), 'The provided context value does not pass validation.');

replaced by the bad context exception tests you wrote in this patch?

On the other hand, something like this looks highly functional:

 // Try to get a value of a valid context, while this value has not been set.
    try {
      $plugin->getContextValue('user');
    }
    catch(ContextException $e) {
      $this->assertIdentical("The entity:user context is required and not present.", $e->getMessage(), 'Requesting a non-set value of a required context should throw a context exception.');
    }

Thinking about this more, since the SimpleTest cannot be removed completely, there's no real reason to mess with it. The unit tests provided here add needed coverage and look good to me.

1. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextDefinitionTest.php
   @@ -0,0 +1,210 @@
   +use Drupal\Core\Plugin\Context\ContextDefinition;
   

   Not used

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextDefinitionTest.php
   @@ -0,0 +1,210 @@
   +      array(NULL, array(), 'nonexistant_constraint_name'),
   

   nonexistent

3. This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary. This will allowed because it is improving test coverage but it nice when someone other than me works this out and adds the info to the issue.

I think this is back to RTBC.

Committed 7acca5b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.