Problem/Motivation

The simpletest \Drupal\content_moderation\Tests\ModerationStateNodeTypeTest::testEnablingOnExistingContent is passing but the very last assertion would fail if it was case insensitive. This is misleading unless you know that by enabling the content_moderation module the value attribute of the publish submit option changes from "Save and publish" to "Save and Publish".

You would be forgiven for believing that the intent of the last assertion was to determine that the text "Save and publish" does not appear in the raw HTML regardless of case.

$this->assertNoRaw('Save and publish');

When attempting to convert this to a BrowserTestBase this fails because assertNoRaw is case insensitive and because the intent of this test is not clear it means we needed clarification before proceeding with the conversion. See: #2864008: Convert web tests to browser tests for content_moderation module

Proposed resolution

Improve the test by adding a comment and perhaps be more explicit that the intent is to prove that enabling the content_moderation module has had the expected impact on the value attribute of the publish submit button

CommentFileSizeAuthor
#16 2864060-16.patch2.26 KBLendude
#16 interdiff-2864060-15-16.txt1.61 KBLendude
#15 2864060-15.patch2.22 KBLendude
#15 interdiff-2864060-14-15.txt2.06 KBLendude
#14 2864060-14.patch2.21 KBnlisgo
#14 interdiff-2864060-9-14.txt2.28 KBnlisgo
#9 2864060-9.patch730 bytesnlisgo
#2 2864060-2-testonly.patch662 bytesnlisgo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nlisgo created an issue. See original summary.

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
FileSize
2864060-2-testonly.patch662 bytes

This test only patch proves that there is a bug.

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2864060-2-testonly.patch, failed testing.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

If it's case insensitive, it should have passed. "Save and publish" is the button text when content moderation isn't installed and "Save and Publish" is the button text once it is installed, so I think this is correct.

An improvement we could make here is adding a property to our base class called $coreSaveButtonText and $moderationSaveButtonText to make it clear what the difference in case for these strings mean.

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented

@Sam152, if this is the case then we should make it more explicit in a comment above the insertion because without that knowledge that the case changes after content_moderation is installed then a developer is going to assume that the intent of the test is to check that "save and publish" is not found in the html regardless of case.

I will refactor the patch in #2864008-2: Convert web tests to browser tests for content_moderation module so that it performs a case sensitive check.

Why is the text changed after content_moderation is installed????

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented

@Sam152, another suggestion would be to check to refactor the test as follows:

// Comment still needed.
$this->assertRaw('Save and Publish');
$this->assertNoRaw('Save and publish');

Thoughts?

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

I like the sounds of #7.

Node: #2753717: Add select field to choose moderation state on entity forms also changes this part of the test, so not sure if it's worth just waiting for that to get in?

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
Title: ModerationStateNodeTypeTest::testEnablingOnExistingContent case sensitive test is passing but it would fail if the test was case insensitive » ModerationStateNodeTypeTest::testEnablingOnExistingContent assertion is misleading
Assigned: nlisgo » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2864060-9.patch730 bytes

Have improved the title and description of this issue. The patch implements the suggestion in #7.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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

Hmmm... I'm not sure that this is right I think the intention of the test was to ensure that a user without the 'use editorial transition publish' couldn't use the publish transition. However the assertion broke when we did #2779647: Add a workflow component, ui module, and implement it in content moderation.

I think we should create a user without that permission instead of doing $this->drupalLogin($this->adminUser); and then test for "Save and Publish" not being there. And then we should grant the user that permission and test that is. This way we'd prevent a similar future regression.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented
Related issues: +#2849753: Determine case sensitivity issues with BTB text assertions
nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented

@alexpott thanks for the clarification. We will need to modify the title and description again. I will try and give this some attention today.

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
Status: Needs work » Needs review
FileSize
interdiff-2864060-9-14.txt2.28 KB
2864060-14.patch2.21 KB

Addressing feedback in #11.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Noctilaris commented
FileSize
interdiff-2864060-14-15.txt2.06 KB
2864060-15.patch2.22 KB

The whole Role thing seems a little convoluted. Why not just create two users like this?

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Noctilaris commented
FileSize
interdiff-2864060-15-16.txt1.61 KB
2864060-16.patch2.26 KB

Cleaned up the naming a bit.

The last submitted patch, 15: 2864060-15.patch, failed testing.

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented

@Lendude, I did wonder whether that would be a better approach. The test case certainly looks a lot cleaner.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Much needed fix, looks good.

The last submitted patch, 15: 2864060-15.patch, failed testing.

alexpott’s picture

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

Committed and pushed 73b750b to 8.4.x and deba11f to 8.3.x. Thanks!

  • alexpott committed 73b750b on 8.4.x 
    Issue #2864060 by nlisgo, Lendude, alexpott: ModerationStateNodeTypeTest...

  • alexpott committed deba11f on 8.3.x 
    Issue #2864060 by nlisgo, Lendude, alexpott: ModerationStateNodeTypeTest...

Status: Fixed » Closed (fixed)

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