Must verify:

  1. verify BigPipeResponseAttachmentsProcessor decorates HtmlResponseAttachmentsProcessor transparently, even when there are more custom attachment types
  2. the passed in response object is not modified
  3. HtmlResponseAttachmentsProcessor's exception in case of a response other than a HtmlResponse still works
CommentFileSizeAuthor
#13 interdiff.txt4.76 KBWim Leers
#12 2674126-12.patch9.53 KBWim Leers
#11 interdiff.txt1.35 KBWim Leers
#10 2674126-10.patch9.22 KBWim Leers
#9 interdiff.txt1.63 KBWim Leers
#8 2674126-8.patch9.4 KBWim Leers
#5 2674126-5.patch7.82 KBWim Leers
#5 interdiff.txt861 bytesWim Leers
#3 2674126-3.patch7.81 KBWim Leers
#2 2674126-2.patch7.68 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2674126-2.patch7.68 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
2674126-3.patch7.81 KB

Still 2 passing tests, but we already had 2 before this patch. So something is wrong.

Turns out the test is in the wrong location and has a broken namespace. This is what you get when you run the test using PHPStorm directly.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Well, WTF, that also didn't work. Even though php vendor/bin/phpunit -c core --testsuite unit --group big_pipe does work locally. Further investigating.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt861 bytes
2674126-5.patch7.82 KB

My bad again. I'm clearly too tired.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Also: none of this would have happened if run-tests.sh had the same behavior as the PHPUnit test runner. This is so very confusing.

Status: Needs review » Needs work

The last submitted patch, 5: 2674126-5.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
2674126-8.patch9.4 KB

Yay, finally!

So, this test coverage uncovered a bug in BigPipe: the requirement the passed in response object is not modified as defined by the \Drupal\Core\Render\AttachmentsResponseProcessorInterface was not being respected. Yay for uncovering that.

Fix attached.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt1.63 KB

d.o lost the interdiff for #8.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
2674126-10.patch9.22 KB

Clean-up.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt1.35 KB

d.o ate my interdiff again. WTF.

This is the interdiff for #10. Simply removes LoC that are no longer needed.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
2674126-12.patch9.53 KB

This improves the legibility/maintainability of this test.

(This time not unchecking the Display checkbox for the interdiff, hoping that the interdiff will not magically disappear this time…)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt4.76 KB

Nope, that didn't help either. This is absolutely maddening.

I reported this to @drumm and @Mixologic in the #drupal-infrastructure IRC channel.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

RTBC - Looks great to me!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Fixed

Perfect, thanks for the review!

  • Wim Leers committed 2c0461a on 8.x-1.x 
    Issue #2674126 by Wim Leers, Fabianx:...

Status: Fixed » Closed (fixed)

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