Problem/Motivation

Shortcut is being removed. We should not depend on that in BigPipe testing. BigPipePlaceholderTestCases.php:148 onwards tests a shortcut module path.

Steps to reproduce

Proposed resolution

Do not depend on shortcut paths in Bigpipe.

Additionally it turned out that even though the shortcut path was tested, shortcut was not even enabled and due to lack of testing of the embeddedHtmlResponse this was masked. So adding additional assertions for that too.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

LLM disclosure

LLM was used to identify and resolve this issue, was human reviewed though.

CommentFileSizeAuthor
#4 3614948-nr-bot_5fwyd4xw.txt1.99 KBneeds-review-queue-bot

Issue fork drupal-3614948

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

gábor hojtsy created an issue. See original summary.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs review

gábor hojtsy opened merge request !16553

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3614948-nr-bot_5fwyd4xw.txt1.99 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Needs review
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Intriguingly I did not find where the test enabled the Shortcut module to begin with, so this may be a "false pass" either way :) Could use review from someone that knows about bigpipe :)

smustgrave’s picture

smustgrave commented

From what I can tell testBigPipe never checks the contents of the embeddedHtmlResponse from BigPipePlaceholderTestCases. So it could be anything and it probably won't fail. Do we leave as is with this fix or update the tests to do more?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Added an assertion method for the hash and embeddedHtmlResponse, let's see how this works.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Looking good, super small comments on the MR.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review

Resolved those. Awaiting fresh test results :) Hope no random fails :D

smustgrave’s picture

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

Those JavaScript tests are everywhere. But feedback appears to be addressed here. Thanks!

  • catch committed f6c662b6 on 11.x 
    task: #3614948 Remove use of Shortcut from BigPipePlaceholderTestCases...

  • catch committed 3f1a3aca on main 
    task: #3614948 Remove use of Shortcut from BigPipePlaceholderTestCases...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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