In #2735199: Convert web tests to browser tests for help module a new method assertNoEscaped() was added to the AssertLegacyTrait trait. The method is currently public, but it should be protected to be consistent with the other assertions in these traits.

This is currently causing BTB tests to fail in contrib. See for example this test for Organic Groups, it fails with the following error now:

Fatal error: Access level to Drupal\simpletest\AssertContentTrait::assertNoEscaped() must be public (as in class Drupal\Tests\BrowserTestBase) in /home/travis/build/amitaibu/og/og_ui/tests/src/Functional/BundleFormAlterTest.php on line 24

CommentFileSizeAuthor
#19 2752315-19-even-better.patch617 byteseric_a
#19 2752315-19.patch615 byteseric_a
#2 2752315-2.patch614 bytespfrenssen

Comments

pfrenssen created an issue. See original summary.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Active » Needs review
StatusFileSize
new 2752315-2.patch614 bytes

Simple 1 line fix, similar to the one from #2744089: Fix visibility of AssertLegacyTrait::assertLink().

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quickfix

+1 I've seen another issue about that though already

pfrenssen’s picture

pfrenssen
Location Sofia
commented

Oh yes indeed this was already reported by @RoySegall, who was also hacking on OG: #2751711: Fix visibility of AssertContentTrait::assertNoEscaped(), I didn't see that.

I'm leaving this up since here the visibility is consistent with the others, while in Roy's patch it is inverted.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Related issues: +#2751711: Fix visibility of AssertContentTrait::assertNoEscaped()
pfrenssen’s picture

pfrenssen
Location Sofia
commented

If this gets committed, please add commit credit to @RoySegall.

  • catch committed 58c3b96 on 8.2.x 
    Issue #2752315 by pfrenssen, RoySegall: Fix visibility of...

catch credited RoySegall.

catch credited RoySegall.

catch’s picture

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

Committed/pushed to 8.2.x, thanks!

eric_a’s picture

eric_a commented
Version: 8.2.x-dev » 8.1.x-dev

Now that #2735199: Convert web tests to browser tests for help module made it into 8.1.x, this is needed too.

eric_a’s picture

eric_a commented
Status: Fixed » Reviewed & tested by the community

RTBC as per #3.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2752315-2.patch, failed testing.

eric_a’s picture

eric_a commented
Status: Needs work » Reviewed & tested by the community

18:33:40 Write error
18:33:40 An error was encountered while attempting to write https://www.drupal.org/files/issues/2752315-2.patch to /var/lib/drupalci/web/jenkins-default-160325

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2752315-2.patch, failed testing.

eric_a’s picture

eric_a commented

Sigh, I requested a standard retest for 8.1.x and it got queued against 8.2.x.

The last submitted patch, 2: 2752315-2.patch, failed testing.

The last submitted patch, 2: 2752315-2.patch, failed testing.

eric_a’s picture

eric_a commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2752315-19.patch615 bytes
new 2752315-19-even-better.patch617 bytes

Ok, a cherry-pick worked, but the patch failed to apply with git apply because the white space context is not in 8.1.x, which did not get the coding standards fix that was added on commit to 8.2.x.
Here are two patches: a rerolled one with the actual context instead of the white space and one that additionally adds the coding standards fix to 8.1.x.
The first is RTBC per #3 and the other per the code snippet in #2735199-19: Convert web tests to browser tests for help module...

pfrenssen’s picture

pfrenssen
Location Sofia
commented

RTBC+1 for the 2nd patch of #19.

catch’s picture

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

Committed/pushed to 8.1.x, thanks!

  • catch committed b36b4f4 on 8.1.x 
    Issue #2752315 by Eric_A, pfrenssen, RoySegall: Fix visibility of...

Status: Fixed » Closed (fixed)

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