Comments

nlisgo created an issue. See original summary.

nlisgo’s picture

Status: Active » Needs review
FileSize
3.72 KB

I have fixed all but one of the failing tests. I believe the failing test is evidence that there is a bug in core. The test passed in simpletest because it was checking for text by doing a case sensitive search:

// \Drupal\Tests\content_moderation\Functional\ModerationStateNodeTypeTest::testEnablingOnExistingContent
$this->assertNoRaw('Save and publish');

The above assertion is failing because the raw text "Save and Publish" is found as assertNoRaw is case insensitive with BrowserTestBase.

I will create a separate issue and amend the test so that we can prove it is broken. I will relate that test to this one once it is created.

Status: Needs review » Needs work

The last submitted patch, 2: convert_web_tests_to-2864008-2.patch, failed testing.

nlisgo’s picture

nlisgo’s picture

I'm going to wait for feedback on #2864060: ModerationStateNodeTypeTest::testEnablingOnExistingContent assertion is misleading and if there is a quick resolution there then I will pick this up again and try and push for a complete conversion of simpletests. Otherwise I will remove the \Drupal\content_moderation\Tests\ModerationStateNodeTypeTest conversion so we can get some of the conversions through and then create a followup issue for #2864060: ModerationStateNodeTypeTest::testEnablingOnExistingContent assertion is misleading

nlisgo’s picture

It appears that the intent of the test was to do a case sensitive check. See: #2864060-5: ModerationStateNodeTypeTest::testEnablingOnExistingContent assertion is misleading.

"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.

We should supply a patch that performs a case sensitive check for the test "Save and publish"

Eric_A’s picture

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Needs work » Needs review
FileSize
733 bytes
4.04 KB
timmillwood’s picture

Looks good apart from the ModerationStateNodeTypeTest change. I think #2864060: ModerationStateNodeTypeTest::testEnablingOnExistingContent assertion is misleading needs committing first.

nlisgo’s picture

It's my view that neither issue need hold up the other. If #2864060: ModerationStateNodeTypeTest::testEnablingOnExistingContent assertion is misleading gets committed first we can reroll and refactor this patch and vice versa.

This patch provides the same level of coverage as is in core currently.

But let's hope #2864060: ModerationStateNodeTypeTest::testEnablingOnExistingContent assertion is misleading gets committed soon and we don't even need to think about it :)

nlisgo’s picture

Duplicate of last message. Not sure how to delete.

dawehner’s picture

I think especially given this patch is mostly about file moving it would probably still apply. Blocking something for the sake of it feels always like an abuse of the system.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Ok, let's RTBC this and get it in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: convert_web_tests_to-2864008-8.patch, failed testing.

nlisgo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: convert_web_tests_to-2864008-8.patch, failed testing.

nlisgo’s picture

Issue tags: +Needs reroll
nlisgo’s picture

Assigned: Unassigned » nlisgo
nlisgo’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.76 KB
timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC if testbot agrees.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: convert_web_tests_to-2864008-19.patch, failed testing.

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

Test fail looks to be unrelated.

michielnugter’s picture

Issue tags: +phpunit initiative

  • catch committed 9e7f7cd on 8.4.x
    Issue #2864008 by nlisgo: Convert web tests to browser tests for...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

catch’s picture

  • alexpott committed 047a7ce on 8.3.x authored by catch
    Issue #2864008 by nlisgo: Convert web tests to browser tests for...

Status: Fixed » Closed (fixed)

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