Please forgive my utter paranoia...

I have been burned recently by trivial composer.json bugs stopping clauses I thought were active from being employed.

I am almost certain - this could not cause problems - but it is better to fix it than worry about the consequences.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug: While the main thrust of this issue is to add integration test - techinically it also a bug fix with a regression test.
Issue priority Not critical because ... they system works as it is.
Unfrozen changes Unfrozen because it add an integration test see /core/tests/Drupal/Tests/ComposerIntegrationTest.php
Prioritized changes The main goal of the issue is integration testing.
CommentFileSizeAuthor
#18 drupal_2472269_8.patch2.8 KBmartin107
#18 justTest-2472269-18.patch2.17 KBmartin107
#8 interdiff-5-8.txt405 bytesderhasi
#8 drupal_2472269_8.patch2.8 KBderhasi
#6 drupal_2472269_5.patch2.78 KBmartin107
#6 interdiff-3.-5.txt799 bytesmartin107
#3 drupal_2472269_3.patch2.76 KBxano
#2 drupal_2472269_2.patch2.77 KBxano
#1 drupal_2472269_1.patch640 bytesxano
suggest-0.patch460 bytesmartin107

Comments

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Title: Trivial syntax error in composer.json » Syntax errors in plugin component composer.json
StatusFileSize
new drupal_2472269_1.patch640 bytes

I found another one.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
StatusFileSize
new drupal_2472269_2.patch2.77 KB

And now with tests.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
StatusFileSize
new drupal_2472269_3.patch2.76 KB

Fixed a copy/paste error.

The last submitted patch, 2: drupal_2472269_2.patch, failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

It didn't fail. I just cancelled it.

martin107’s picture

martin107 commented
StatusFileSize
new interdiff-3.-5.txt799 bytes
new drupal_2472269_5.patch2.78 KB

@Xano -Thank you :-) I will be smiling from ear to ear all day.

Yes Yes Yes fix the issue now and always before commit... I must adjust my perspective.

On a minor note I ran the new file through phpcs and found a couple of ultra trivial nits.

martin107’s picture

martin107 commented
Priority: Minor » Normal
Issue summary: View changes
Issue tags: -Quick fix

first patch was adding a single comma, and could be justified as a minor quickfix

Adding beta evaluation

derhasi’s picture

derhasi commented
StatusFileSize
new drupal_2472269_8.patch2.8 KB
new interdiff-5-8.txt405 bytes
  • The test seems not be run by testbot: see https://qa.drupal.org/pifr/test/1024873, there is not call to ComposerIntegrationTest.
  • Locally, when using Testing module, the Test also does not appear.

After adding @group Composer to the class, it appears in the PHPUnit section. (The group seems not to have a effect on the UI, is there an issue for that?)

So I reapplied the patch with the given group annotation, so hopefully it will be recognized by the testbot too.

  • The local test run was succesful.
  • Also composer validate for the changed composer.json works does not fail anymore too.
derhasi’s picture

derhasi commented
Issue tags: +drupaldevdays, +Composer
xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

The testbot picked up the test. Thanks!

derhasi’s picture

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

As this was just an issue of the testbot, I think it is fine to RTBC, as the patch and test itself work nicely.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Parent issue: » #2337283: Add a composer.json file to every component

Some similar work: #2337283: Add a composer.json file to every component

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

+1 RTBC. The test makes us happy. :-)

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Syntax errors in plugin component composer.json » Fix syntax errors in Drupal\Component\Plugin's composer.json
eclipsegc’s picture

eclipsegc commented

Just ++ing the rtbc here.

Eclipse

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Fix syntax errors in Drupal\Component\Plugin's composer.json » Fix syntax errors in Drupal\Component\Plugin's composer.json, Add test
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

So it looks like the added test did indeed get run in https://qa.drupal.org/pifr/test/1025348, yay! However I'm unclear on what the earlier testbot issue was. There also is no test-only patch on this issue exposing the fails and that the test runs alright without the fix being there. (See https://www.drupal.org/core-gates#testing).

Can someone resubmit the patch as a test-only patch followed by a combined patch, to expose the test coverage? Thanks!

martin107’s picture

martin107 commented
StatusFileSize
new justTest-2472269-18.patch2.17 KB
new drupal_2472269_8.patch2.8 KB

That is a good way to build confidence.

The last submitted patch, 18: justTest-2472269-18.patch, failed testing.

martin107’s picture

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

  • xjm committed d51d223 on 8.0.x 
    Issue #2472269 by martin107, Xano, derhasi: Fix syntax errors in Drupal\...
xjm’s picture

xjm
she/her
Primary language English
commented
Component: plugin system » base system
Status: Reviewed & tested by the community » Fixed

Thanks @martin107, that's a big help. So testbot does indeed properly report the failure in #18. And we can also confirm it's detecting the syntax error by applying the test-only patch locally and running it:

1) Drupal\Tests\ComposerIntegrationTest::testComposerJson
Syntax error

/Users/jess.myrbo/git/drupal/core/tests/Drupal/Tests/ComposerIntegrationTest.php:66
                                     
FAILURES!                            
Tests: 1, Assertions: 0, Failures: 1.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Thanks everyone! Also thanks for the beta evaluation.

Status: Fixed » Closed (fixed)

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

davidwbarratt’s picture

davidwbarratt commented
Related issues: +#2545344: Drupal\Tests\ComposerIntegrationTest assumes there is a composer.json in DRUPAL_ROOT

This change made unit tests fail for me, #2545344: Drupal\Tests\ComposerIntegrationTest assumes there is a composer.json in DRUPAL_ROOT