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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Active » Needs review
FileSize
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
CreditAttribution: DuaelFr 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
CreditAttribution: Mile23 commented
Issue summary: View changes
FileSize
2375737_3.patch7.32 KB
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
CreditAttribution: DuaelFr 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
CreditAttribution: Mile23 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
CreditAttribution: Mile23 commented
Status: Needs work » Needs review

Testbot is happy again.

daffie’s picture

daffie CreditAttribution: 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 🇨🇦
CreditAttribution: webchick 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.