In \Drupal\Tests\workspaces\Functional\WorkspaceTestUtilities, ::createAndActivateWorkspaceThroughUi() and ::createWorkspaceThroughUi() both have a line without side effects whose return value isn't used:

$this->getSession()->getPage()->hasContent("$label ($id)");

Unless it was meant to throw some kind of exception or something, I assume we can remove it.

Issue fork drupal-3506527

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

traviscarden created an issue. See original summary.

traviscarden opened merge request !11203

traviscarden’s picture

traviscarden commented
Assigned: traviscarden » Unassigned
Status: Active » Needs review
Issue tags: +Quick fix
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

There are several instances of hasContent() being used this way.

So if actually a bug, not 100% sure it is, think we should address all instances then.

quietone’s picture

quietone commented
Version: 11.1.x-dev » 11.x-dev

It would help to check the issue when that line was introduced. Maybe it was to do something?

traviscarden’s picture

traviscarden commented
Status: Needs work » Needs review

Well, the oldest instance in the file came into Core that way--#2784921: Add Workspaces experimental module--and the second one looks like a copy/paste of the first one. Going back to the contrib module, it looks like it may be refactoring artifact from a very similar line at the same place in the code that actually had side effects:

 $this->getSession()->getPage()->findButton($workspace->label())->click();

Compare the Core code in question:

$this->getSession()->getPage()->hasContent("$label ($id)");

It used to be doing something--clicking. Now it's just an unused query. I do think it's just dead code.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

If that’s the case there is dead code throughout where similar code is used. So if it’s dead code should address all instances

traviscarden’s picture

traviscarden commented
Title: Dead code in WorkspaceTestUtilities » Remove dead code in various test classes
Status: Needs work » Needs review

Huh, you're right. Curious. Well, this gets all the other instances I see.

amateescu’s picture

amateescu commented
Component: workspaces.module » other

Wouldn't it make more sense to wrap those lines with $this->assertTrue() calls?

traviscarden’s picture

traviscarden commented

Well, let's try it and see if the tests pass, shall we?

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Pipeline has failed with following error,

Behat\Mink\Exception\ResponseTextException: The text "Has taxonomy term (= term1)" was not found anywhere in the text of the current page.

.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.