While I was doing some work on a node access issue, I noticed that the node access pager test we added in #681760: Try to improve performance and eliminate duplicates caused by node_access table joins could use a little cleanup for easier debugging. The attached patch clarifies some assertion messages and comments, and removes assertion messages that hide the more specific message provided by the base class method.

CommentFileSizeAuthor
#8 node-test-cleanup-1786142-8.patch1.98 KBdcam
#5 1786124-5.patch2.12 KBxjm
#5 interdiff.txt765 bytesxjm
node-access-pager-cleanup.patch2.08 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

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

quoting xjm from IRC: "the logic is that assertText() and assertRaw() provide their own assertion messages that are much more specific" and that's why those get messages removed the others just the t().

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Reviewed & tested by the community » Needs work

A minor thing: the first occurrence of "Lookup" was replaced with "Look up" but the second was left as-is, they should both either be left as-is or changed to the same values; fyi "lookup" is a valid word so I'd question the need to change it at all.

Beyond that, the patch works fine and does exactly what it says it does - the messages from the base class are displayed instead of the previously overridden values.

xjm’s picture

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

Actually, that's incorrect. "Lookup" is a noun. "Look up" is a verb. :)

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Reviewed & tested by the community » Needs work

Per irc, there's a second occurrence of "lookup" :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
interdiff.txt765 bytes
1786124-5.patch2.12 KB

I get it now. Here's that fixed as well. Thanks @DamienMcKenna!

Also tagging for backport for parity.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Needs review » Reviewed & tested by the community

Setting it back to RTBC based because chx liked it and my own review.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Yep. Committed/pushed to 8.x.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Patch (to be ported) » Needs review
FileSize
node-test-cleanup-1786142-8.patch1.98 KB

Backported #5 to D7.

xjm’s picture

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

Looks good, thanks @dcam!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x, too. Thanks!

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