Problem/Motivation

Not enough coverage for Drupal\Component\Plugin.

ReflectionFactory has a CRAP score of 110, which means it's a project risk within its namespace.

Along with the others in #2052109: [meta] Expand phpunit tests for \Drupal\Component\Plugin classes it should be unit tested.

Proposed resolution

Write some tests.

Make a patch.

Commit the patch.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only improves automated testing.
CommentFileSizeAuthor
#3 interdiff.txt2.26 KBmile23
#3 2375737_3.patch7.32 KBmile23
#1 2375737_1.patch6.68 KBmile23

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 2375737_1.patch6.68 KB

A patch. Covers ReflectionFactory 100%, reduced CRAP score to 10.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented

12/12 tests PASS
100% coverage !

Good job :)
But...

+++ b/core/tests/Drupal/Tests/Component/Plugin/Factory/ReflectionFactoryTest.php
@@ -0,0 +1,188 @@
+class ArgumentsPluginId {
...
+  public function __construct($plugin_id) {
...
+class ArgumentsMany {
...
+  public function __construct(
...
+class ArgumentsConfigArrayKey {
...
+  public function __construct($config_name) {
...
+class ArgumentsAllNull {
...
+  public function __construct($charismatic, $demure, $delightful, $electrostatic) {
...
+class ArgumentsNoConstructor {

Missing tiny docblocks? :p

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
StatusFileSize
new 2375737_3.patch7.32 KB
new interdiff.txt2.26 KB

Thanks!

Added docblocks. Also changed the docblock for StubReflectionFactory just a little, and also the return line of getPluginClass().

Also added beta evaluation.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Needs review » Reviewed & tested by the community

I don't think we absolutely need docblocks on the empty constructors for your stub classes so, let's RTBC.
Good job.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2375737_3.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Not really sure why the test fails, especially with just 'Terminated' written over and over in the log.

The tests added here all pass locally, so I'm re-queuing.

Mile23 queued 3: 2375737_3.patch for re-testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Testbot is happy again.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

It was RTBC before the failed retest and nothing has changed, so back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Not really the most qualified to give these sign-off, but they've been sitting here long enough for someone to raise objections, and moar test coverage is never a bad thing.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed a850b25 on 8.0.x 
    Issue #2375737 by Mile23: Expand unit testing for Drupal\Component\...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.