Issue #2827721 by mglaman: Expand test coverage for add to cart forms. #756
Conversation
|
This needs matching Javascript test. |
mglaman
force-pushed
the
2827721-add-to-cart-form-id
branch
from
6206312 to
5edc808
Compare
| * {@inheritdoc} | ||
| */ | ||
| public function buildForm(array $form, FormStateInterface $form_state) { | ||
| // Sleep between 0.0 and 2.0 seconds. |
There was a problem hiding this comment.
Why make this random? Why not just always 2 seconds? Or 1?
There was a problem hiding this comment.
According to https://www.drupal.org/node/2886614, there is a chance the forms could be built out of order. To ensure unique form IDs, we ended up using a static counter. If the forms are built out of order, this causes a bug in form submission. My goal is to reproduce the bug and test our assumed fix.
So I thought random times might cause the build to get weird. However by this time the form ID is known... so maybe slowdown happens elsewhere when rendering these formatter placeholders.
| @@ -0,0 +1,12 @@ | |||
| name: Commerce Cart Big Pipe | |||
| @@ -0,0 +1,12 @@ | |||
| name: Commerce Cart Big Pipe | |||
| type: module | |||
| description: Provides a slow version of AddToCart form for testing Big Pipe streaming | |||
| */ | ||
| function commerce_cart_big_pipe_module_implements_alter(&$implementations, $hook) { | ||
| if ($hook == 'entity_type_build') { | ||
| // commerce_cart_big_pipe_entity_type_build() needs to run after |
There was a problem hiding this comment.
Specifying a module weight might be simpler than this.
| $seen_ids[] = $form->getAttribute('id'); | ||
| } | ||
| $this->assertSession()->responseHeaderNotEquals('BigPipe-Test-Placeholders', '<none>'); | ||
| $this->assertSession()->responseHeaderEquals('BigPipe-Test-No-Js-Placeholders', '<none>'); |
There was a problem hiding this comment.
I'm not sure why you're testing these two things?
There was a problem hiding this comment.
And if you are, you probably want to test the specific value of this header?
There was a problem hiding this comment.
It was some debugging, basically. But I am going to test the header, now!
| /** | ||
| * Tests that the form IDs are unique. | ||
| */ | ||
| public function testUniqueAddToCartFormIds() { |
There was a problem hiding this comment.
I think this test method is meant to test BigPipe-powered delivery?
I replied to your question on Twitter (https://twitter.com/wimleers/status/883262976576413696), but repeating here to prevent linkrot:
Recommendation:
- assert HTML placeholder
- wait for the BigPipe stop signal
- assert HTML placeholder has been replaced
See
\Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest::testMultipleClosingBodies_2678662()— waiting for the stop signal
There was a problem hiding this comment.
With previous comment, the original issue was ensuring we had unique form IDs, then I also used it to debug BigPipe delivery.
mglaman
force-pushed
the
2827721-add-to-cart-form-id
branch
from
fa7ffb3 to
cd17262
Compare
|
Committed in f37464b |
2 participants
No description provided.