Problem/Motivation

There's a double $this->drupalGet('admin/structure/types/manage/moderated'); in core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTypeTest.php

Discovered in #2937515: Fix Drupal.Array.Array.[ArrayClosingIndentation, ArrayIndentation] coding standard https://git.drupalcode.org/project/drupal/-/merge_requests/515#note_36507

Steps to reproduce

Proposed resolution

Remove Remove the the unneeded unneeded double double $this->drupalGet $this->drupalGet

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3226516

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

Spokje created an issue. See original summary.

spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes

Spokje opened merge request !1001

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Thinking out loud: should we try to detect when this happens in drupalGet() itself? Is there ever a case where you might want to get the same page twice in a row?

spokje’s picture

spokje
Primary language Dutch
commented

Thinking out loud: should we try to detect when this happens in drupalGet() itself? Is there ever a case where you might want to get the same page twice in a row?

The only thing (how unlikely it is) I can come up with is, we need to test something like: "The issue only happens on a reload of the form without doing anything to it."

Looks impossible to me, but I'm getting less and less surprised over time being involved in BugSmash.... :)

I would not assume anything in drupalGet() to detect it's multiple-called:

1) The unlikely _might_ happen
2) I think test-functionality should be as unopinionated/dumb as possible
3) Double calling doesn't break anything (at the very least in this test)

drops EUR 0.02

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I was thinking from the point of view of optimising test performance but as you pointed out this has no side effects and there might be a valid case somewhere so not really worth doing anything about it.

  • catch committed 6f3a08f on 9.3.x 
    Issue #3226516 by Spokje, longwave: Unneeded double $this->drupalGet in...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thinking out loud: should we try to detect when this happens in drupalGet() itself? Is there ever a case where you might want to get the same page twice in a row?

I think we do that in page cache tests - first time to confirm a cache miss second to confirm a cache hit.

Committed 6f3a08f and pushed to 9.3.x. Thanks!

Spokje closed merge request !1001

Status: Fixed » Closed (fixed)

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