Problem/Motivation

We left behind some tests extending ContentTranslationUITestBase by mistake.

Proposed resolution

Fix them

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#13 2948073-2.patch1.36 KBAnonymous (not verified)
#9 interdiff-2-9.txt2.52 KBAnonymous (not verified)
#9 2948073-9.patch2.62 KBAnonymous (not verified)
#6 interdiff-2-6.txt821 bytesAnonymous (not verified)
#6 2948073-6.patch2.16 KBAnonymous (not verified)
#5 2948073-4.patch2.62 KBAnonymous (not verified)
#2 2948073-2.patch1.36 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
StatusFileSize
new1.36 KB

Whoops.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

run-tests.sh--

Anonymous’s picture

This is strange, but these tests fail locally for me with:

Object of class Drupal\Core\StringTranslation\TranslatableMarkup could not be converted to int

That's what it took to pass. I don't know why.

Anonymous’s picture

StatusFileSize
new2.62 KB
Anonymous’s picture

StatusFileSize
new2.16 KB
new821 bytes

Well, this is due to the fact that WebAssert::responseContains() should take a string. But AssertLegacyTrait::assertRaw() sends there the object in this case. Next fix also works.

alexpott’s picture

I think I prefer #5 as it makes conversions easier. Since you don't need to think about the object to string again. Can you re-upload that with an interdiff to #2? Thanks!

Anonymous’s picture

PhpStrorm found 718 usages of AssertLegacyTrait::assertRaw (Ctrl+B by method). Many of them using t() value. I tried to run some of them - and they also fail (but pass with fix from #6).

But why this does not happen on DI?

Also in Drupal\KernelTests\AssertContentTrait we have

protected function assertRaw($raw, $message = '', $group = 'Other') {
  ...
  return $this->assert(strpos($this->getRawContent(), (string) $raw) ...
}

protected function assertNoRaw($raw, $message = '', $group = 'Other') {
  ...
  return $this->assert(strpos($this->getRawContent(), (string) $raw)...
}

So, maybe (string) $raw will give more compatibility between KernelTests\AssertContentTrait and FunctionalTests\AssertLegacyTrait?

Anonymous’s picture

Status: Reviewed & tested by the community » Needs review
Related issues: +#2549805: [Meta] Remove all usage of FormattableMarkup in tests apart from explicit tests of that API
StatusFileSize
new2.62 KB
new2.52 KB

#7: Done.

alexpott’s picture

@vaplas it does not happen for me locally either. It'd be good to understand why your set up is experiencing this problem. Since #2 passes locally for me and the testbot.

Anonymous’s picture

The problem in the latest version of the Mink. After https://github.com/minkphp/Mink/pull/743.

public function responseContains($text) {
...
-  $regex = '/'.preg_quote($text, '/').'/ui';
-  $this->assert((bool) preg_match($regex, $actual), $message);
+  $this->assert(strpos($actual, $text) !== FALSE, $message);
}

Current versioin:
$this->assert(stripos($actual, $text) !== false, $message);

Edit:

  • Before $text casting to string.
  • After $text casting to int (like needle parameter of the strpos/stripos)
alexpott’s picture

@vaplas so I think we should back out your changes and file a separate issue about the changes to Mink and perhaps a PR against mink to fix unless this is a major version thing because they've changed their API.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new1.36 KB

Yep, sorry for noise about it here. Back to #3 RTBC.

Anonymous’s picture

#12.2 #2948700: Casting $text value to string in responseContains/responseNotContains methods.

So, please do not add credit to me here. The patch does not contain my work. My credit is waiting for me in #2948700 :)

alexpott’s picture

@vaplas I think you do deserve credit on this issue. Credit is not just about whether you wrote lines in the patch. Your review and work discovered a problem that we have to address in another issue. Yes you'll get credit in that issue but your work here is valuable too.

catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed 0a97488 on 8.6.x
    Issue #2948073 by vaplas, alexpott: ContentTranslationUITestBase...

  • catch committed 6468b98 on 8.5.x
    Issue #2948073 by vaplas, alexpott: ContentTranslationUITestBase...

Status: Fixed » Closed (fixed)

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