It seems that the code below changed in #1599108: Allow modules to register services and subscriber services (events) has caused an issue with running simpletests in the browser. For example the Theme group of tests has 4 failures.

function current_path() {
  // @todo Remove the check for whether the request service exists and the
  // fallback code below, once the path alias logic has been figured out in
  // http://drupal.org/node/1269742.
  if (drupal_container()->has('request')) {
    return drupal_container()->get('request')->attributes->get('system_path');
  }
  // If we are outside the request scope, fall back to using the path stored in
  // _current_path().
  return _current_path();
}

This test... (core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php)

  /**
   * Ensure page-front template suggestion is added when on front page.
   */
  function testFrontPageThemeSuggestion() {
    $original_path = _current_path();
    // Set the current path to node because theme_get_suggestions() will query
    // it to see if we are on the front page.
    config('system.site')->set('page.front', 'node')->save();
    _current_path('node');
    $suggestions = theme_get_suggestions(array('node'), 'page');
    // Set it back to not annoy the batch runner.
    _current_path($original_path);
    $this->assertTrue(in_array('page__front', $suggestions), t('Front page template was suggested.'));
  }

... does not work because drupal_is_front_page() will return false because although the test has called _current_path('node') to fake the path when drupal_is_front_page() calls current_path() it returns 'batch' because that is the url that's running the tests :)

I've marked this as critical because (a) it's a regression (b) the work-around of using run-tests.sh isn't exactly user friendly for people new to testing and contributing to core.

CommentFileSizeAuthor
#5 1719936-5.drupal8.theme-tests.approach2.patch3.02 KBalexpott
#5 0-5-interdiff.txt606 bytesalexpott
#3 1719936.drupal8.theme-tests.approach2.patch3.02 KBalexpott
#3 1719936.drupal8.theme-tests.approach1.patch1.1 KBalexpott

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Sorry for putting this in the wscci component - I guess it should have been in base system.

Crell’s picture

Crell commented
Component: wscci » base system
Issue tags: +WSCCI

The wscci component is rather vestigial at this point...

This is yet another case where we're uncovering misbehaving spaghetti messing with global state when we go and rip out the global state. :-( My knee-jerk response is that the test needs to be updated. Alexpott, think you can take a crack at it?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 1719936.drupal8.theme-tests.approach1.patch1.1 KB
new 1719936.drupal8.theme-tests.approach2.patch3.02 KB

Approach 1
Fixes _current_path to set the system_path in the request object if in scope. This moves _current_path() to after the class loader has run.

Approach 2
Adds setPath() function to TestBase class that determines if the request object is in scope and sets system_path accordingly.

katbailey’s picture

katbailey commented
Status: Needs review » Needs work 
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -939,4 +939,23 @@ abstract class TestBase {
+   * Useful for tests that rely on functions the require current_path() to

Should be "functions that require"

For a quick solution to this, approach 2 is far preferable as it only touches test code. I don't like the fact that it means messing with the request object of the request that's actually running the test, but I can't think of another way around this - at least not one that doesn't require a complete overhaul of WebTestBase ;-)

So yeah, if you want to reroll approach 2 with that tiny little fix, I'll RTBC it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 0-5-interdiff.txt606 bytes
new 1719936-5.drupal8.theme-tests.approach2.patch3.02 KB

Change from #4 applied

katbailey’s picture

katbailey commented

Sorry, I did more thinking about this yesterday and realised it was tied in with #1727538: WebTest tests should have access to the correct DIC. Once I get a decent patch in place for that, it fixes this issue and is definitely a more correct way to do it. Because that way we ensure that the test case only has access to its own 'testing' container, and is not erroneously being told it can get the system path from the request object of the parent environment.

But if people feel we need a stopgap fix for this then #5 is good to go.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Postponed

The hackiness of the current approach, the stopgap fix, and the promise of the new fix are enough for me to postpone this.

mbrett5062’s picture

mbrett5062 commented

As #1727538: WebTest tests should have access to the correct DIC is in, why has this not been closed out? As @katbailey said in #6 that is the correct fix for this issue. We are currently over limits on critical issues, and this should be closed IMHO.
I am not sure I am allowed to mark as closed, but please if someone could.

katbailey’s picture

katbailey commented
Status: Postponed » Closed (duplicate)

Just an oversight, should have been closed out long ago.