Problem/Motivation

ResponsiveImageFieldUiTest.php is at core/modules/responsive_image/src/FunctionalJavascript/ResponsiveImageFieldUiTest.php.

This test was added in #2809513: Convert AJAX part of \Drupal\responsive_image\Tests\ResponsiveImageFieldUiTest to JavascriptTestBase and the rest to BrowserTestBase.

Shouldn't it be at core/modules/responsive_image/tests/src/FunctionalJavascript/ResponsiveImageFieldUiTest.php?

It also doesn't declare a $defaultTheme. Maybe this has been missed since it's not in the correct directory. Maybe it never even runs! I don't know enough about the test bot to say for sure.

Proposed resolution

Move it! And maybe address the $defaultTheme if it throws a deprecation warning.

I suspect there will indeed be a deprecation warning once this test gets moved to the correct directory due to this issue: #3082655: Specify the $defaultTheme property in all functional tests. There is an associated change record for that as well.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 responsiveimagefield-3248816-12.patch1.06 KBbsuttis
#12 interdiff-3248816-10-12.txt656 bytesbsuttis
#10 interdiff-3248816-7-10.txt678 bytesbsuttis
#10 responsiveimagefield-3248816-10.patch1.06 KBbsuttis
#7 responsiveimagefield-3248816-7.patch842 bytesbsuttis

Comments

danflanagan8 created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Yeah I suspect it's not running

cilefen’s picture

cilefen commented
Issue tags: +Novice

It is possible for a novice contributor to take the next step on this one.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Issue summary: View changes
Related issues: +#2809513: Convert AJAX part of \Drupal\responsive_image\Tests\ResponsiveImageFieldUiTest to JavascriptTestBase and the rest to BrowserTestBase, +#3082655: Specify the $defaultTheme property in all functional tests

Sounds good. I've added a little more context to the IS along with some related issues.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Can we write a test to ensure all our tests are in the right place in future?

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

As long as we don't put that one in the wrong place. ;)

bsuttis’s picture

bsuttis commented
Status: Active » Needs review
StatusFileSize
new responsiveimagefield-3248816-7.patch842 bytes

Here's a patch moving the test to tests/src and I also added $defaultTheme to the test.

Status: Needs review » Needs work

The last submitted patch, 7: responsiveimagefield-3248816-7.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

The namespace line at the top of the test will need to be updated. You can copy off of the other tests that are in the new directory.

bsuttis’s picture

bsuttis commented
Status: Needs work » Needs review
StatusFileSize
new responsiveimagefield-3248816-10.patch1.06 KB
new interdiff-3248816-7-10.txt678 bytes

Thanks, noticed the namespace issue. Here's another go at it.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs review » Needs work 
+++ b/core/modules/responsive_image/tests/src/FunctionalJavascript/ResponsiveImageFieldUiTest.php
@@ -12,6 +12,11 @@
+  protected $defaultTheme = 'stable';

Searching the core codebase, stable is almost never used as the $defaultTheme. I see only one instance. Can we change this to stark to be more in line with current practice?

bsuttis’s picture

bsuttis commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3248816-10-12.txt656 bytes
new responsiveimagefield-3248816-12.patch1.06 KB

Confirming 'stark' is now used.

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

Before applying the patch. Nice to know there is only one misplaced test.

$ find . -path "./core/*" -iname \*Test.php | grep -v tests
./core/modules/responsive_image/src/FunctionalJavascript/ResponsiveImageFieldUiTest.php

After

$ find . -path "./core/*" -iname \*Test.php | grep -v tests
$

And the test appears in the test output, there is only one so we know it is the one that has been moved.

Drupal\Tests\responsive_image\FunctionalJavascript\Responsiv 1 passes
This looks fine to me.

  • catch committed 3795c75 on 9.4.x 
    Issue #3248816 by bsuttis, danflanagan8, quietone:...

  • catch committed 5cf24e8 on 9.3.x 
    Issue #3248816 by bsuttis, danflanagan8, quietone:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.4.x and cherry-picked to 9.3.x, thanks!

Status: Fixed » Closed (fixed)

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