Problem/Motivation

The changes to Drupal\KernelTests\KernelTest->render() in #2564547: Remove calls to drupal_process_attached were incorrect, because both the input and output variables used in the test are null. This will presumably result in this test always passing.

Proposed resolution

Use real values.

Remaining tasks

Write the patch

User interface changes

None - bug fix in tests

API changes

None

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because some tests are not failing when they should
Issue priority Major because other patches might be passing when they should fail.
Unfrozen changes Unfrozen because it only changes tests.
Disruption Not disruptive
CommentFileSizeAuthor
#23 2568511_23.patch7.96 KBWim Leers
#11 2568511_11.patch8.43 KBMile23
#8 interdiff.txt1.36 KBMile23
#8 2568511_8.patch5.4 KBMile23
#5 interdiff.txt3.47 KBMile23
#5 2568511_5.patch5.48 KBMile23
#2 2568511-2-fix-render-test.patch2.01 KBianthomas_uk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ianthomas_uk created an issue. See original summary.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk as a volunteer commented
Assigned: ianthomas_uk » Unassigned
Status: Active » Needs review
FileSize
2568511-2-fix-render-test.patch2.01 KB

Also brought the simpletest version inline with the core/tests version, which was missed in the previous issue.

Status: Needs review » Needs work

The last submitted patch, 2: 2568511-2-fix-render-test.patch, failed testing.

The last submitted patch, 2: 2568511-2-fix-render-test.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2568511_5.patch5.48 KB
interdiff.txt3.47 KB

Ugh.

Nice catch.

Here are some fixes.

We do a check for html_response_attachment_placeholders in HtmlResponseAttachmentsProcessor and add a check in TableTest.

Will need to reroll #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. with this change to HtmlResponseAttachmentsProcessor.

Status: Needs review » Needs work

The last submitted patch, 5: 2568511_5.patch, failed testing.

The last submitted patch, 5: 2568511_5.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2568511_8.patch5.4 KB
interdiff.txt1.36 KB

And fixing TableTest...

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk as a volunteer commented
Status: Needs review » Reviewed & tested by the community

I figure @Mile23 is happy with the changes I made to the two KernelTest::render() methods.

The other changes are his and are sensible. HtmlResponseAttachmentsProcessor is just an extra if (the noise is just because of indenting) and the new TableTest is nicer than the old, as it tests the rendered output rather than the intermediate data structure.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

This will need a reroll after #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2568511_11.patch8.43 KB

And here's your re-roll.

The patch around HtmlResponseAttachmentsProcessor::processAttachment() looks hairy but all it does is bracket the processing by checking whether $attached['html_response_attachment_placeholders'] exists. git apparently can't say 'just indent all this code by 2 spaces.'

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue tags: -Needs reroll
ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Reroll is good. Git might not be able to just indent a block of code, but source tree can ignore white space ;)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2568511_11.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Pretty sure the fate of this patch might be tied up in #2571427: HtmlResponseAttachmentsProcessor tests misplaced during reroll

Not sure if it's postponed or what.

Status: Needs work » Needs review

Mile23 queued 11: 2568511_11.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2568511_11.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review

Failing test is MigrateUserProfileEntityFormDisplayTest, which passes locally with and without the patch in #11.

I'm calling this an unrelated fail.

Fabianx’s picture

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

Looks good to me, looks like a bug fix to me, too.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Fix KernelTestBase::render broken in Remove calls to drupal_process_attached » Fix broken test: KernelTestBase::render
Priority: Normal » Major
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

At first sight, the changes in \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::processAttachments() look excessive.

But on second sight it's just wrapping that logic in an if-test. It's totally harmless and in fact makes the logic a bit easier to follow. I support this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
FileSize
2568511_23.patch7.96 KB

#2555069: Remove invocation of hook_html_head_alter() conflicted with this. Smaller patch, because fewer LoC remain to be wrapped in that if-statement, hence fewer changes.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Discussed with @Wim Leers in IRC - we agreed that this change makes sense is very low risk and not disruptive. Committed a47dd7a and pushed to 8.0.x. Thanks!

  • alexpott committed a47dd7a on 8.0.x 
    Issue #2568511 by Mile23, ianthomas_uk, Wim Leers: Fix broken test:...

Status: Fixed » Needs work

The last submitted patch, 23: 2568511_23.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Fixed

meh

Status: Fixed » Closed (fixed)

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