Problem/Motivation

There is no way to get the drupalSettings as part of the test suite, unlike with the old test suite.

Source: Jaesin

Proposed resolution

Add it back, have a different implementation for JS vs. no JS

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Title: Add a drupalGetSettings method to BTB/JTB » Add a drupalGetSettings method to JTB
Status: Active » Needs review
FileSize
859 bytes
neclimdul’s picture

Hard problem number 2, being as we have different types of settings in Drupal it seems like this should document that this is getting javascript drupalSettings. I couldn't figure out why only JTB cared about Drupal's settings and only after reading the implementation did it make sense. Preferably in the signature I think but something.

dawehner’s picture

Hard problem number 2, being as we have different types of settings in Drupal it seems like this should document that this is getting javascript drupalSettings. I couldn't figure out why only JTB cared about Drupal's settings and only after reading the implementation did it make sense. Preferably in the signature I think but something.

Well, BTB has its own variant already, but its not able to take into account he dynamicness of JTB.

claudiu.cristea’s picture

claudiu.cristea’s picture

BTB has its own variant already, but its not able to take into account he dynamicness of JTB

@dawehner, this is tested now in #5.

dawehner’s picture

Nice @claudiu.cristea
I wouldn't have expected someone to jump in that quick. Awesome!

claudiu.cristea’s picture

@dawehner I was more or less cross-posting. I did the patch before your comment in #4

Jaesin’s picture

I was using this before I knew I could execute JS:

<?php
  /**
   * Gets drupal settings and parses into an php array.
   *
   * @return array
   *   The drupal settings object.
   */
  public function getDrupalSettings() {
    // Get the active drupal settings from the session.
    return Json::decode($this->getSession()->getPage()->find('css', 'script[data-drupal-selector=drupal-settings-json]')->getHtml());
  }
?>
claudiu.cristea’s picture

@Jaesin, a variant of that is now implemented in BrowserTestBase. But that cannot resolve the dynamic case from the test in #5.

dawehner’s picture

Yeah I think this code is just in 8.2.x but not in 8.1.x

claudiu.cristea’s picture

Title: Add a drupalGetSettings method to JTB » Add a getDrupalSettings() method to JTB
dawehner’s picture

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -143,4 +143,15 @@ public function assertSession($name = NULL) {
+    return $this->getSession()->evaluateScript('function(){ if (typeof drupalSettings !== \'undefined\') { return drupalSettings; }}()') ?: [];

Is there any reason we need to less readable JS here? We could also use new lines I guess, right and by doing so, make it more readable.

claudiu.cristea’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks a lot @claudiu.cristea!

  1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
    @@ -143,4 +143,22 @@ public function assertSession($name = NULL) {
    +    $script = <<<EndOfScript
    +(function () {
    +  if (typeof drupalSettings !== 'undefined') {
    ...
    +})()
    +EndOfScript;
    

    <3

  2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
    @@ -143,4 +143,22 @@ public function assertSession($name = NULL) {
    +   }
    

    Nitpick: This line seems to have one space too much :)

neclimdul’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -143,4 +143,22 @@ public function assertSession($name = NULL) {
+   * Gets Drupal settings from session and parses into a PHP array.

Could we mention something about these being Javascript settings? It sounds like they're coming from _SESSION or something when really that's referring to the internal working of mink not what you're retrieving. IE You have to know this method lives on JTB to understand the documentation which isn't really great.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
1.11 KB
neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

awesome!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2787923-17.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

https://www.drupal.org/pift-ci-job/519413 failure is weird. Trying to rerun the test.

cilefen’s picture

Title: Add a getDrupalSettings() method to JTB » Add a getDrupalSettings() method to JavascriptTestBase
claudiu.cristea’s picture

Very small docs fix.

klausi’s picture

+++ b/core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php
@@ -62,4 +62,29 @@ public function testCreateScreenshot() {
+  public function testGetDrupalSettings() {
+    /** @var \Drupal\Core\Extension\ModuleInstallerInterface $module_installer */
+    $module_installer = $this->container->get('module_installer');
+    $module_installer->install(['test_page_test']);

If a test method requires an additional module then this is a good sign that the test method should live on its own test class instead. We should stop accessing the Drupal API altogether in functional tests like that, see also #2783193: Make browser tests use the browser only for assertions.

Leaving at RTBC for now, not sure if that should hold up this patch.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Hmm I think it's worth holding the patch up to be honest. We don't have many js tests yet and most people will add to them by copy/paste/modify, so should try to keep what we have as tidy as possible.

claudiu.cristea’s picture

(...) it's worth holding the patch up

@catch, you mean separate the test in its own class or add the module in BrowserWithJavascriptTest::$modules?

catch’s picture

@claudiu.cristea I don't really mind on that. For test performance a new class is a bit better since we won't install the module unnecessarily for the other methods, so maybe new class?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.72 KB
3.6 KB

Here we go.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice work

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: 2787923-27.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.71 KB
634 bytes

Ouch! the namespace. Setting back to RTBC as per #28.

claudiu.cristea’s picture

Main patch in #30. Adding also a patch for 8.2.x if will ever be considered.

  • catch committed f25e39f on 8.3.x
    Issue #2787923 by claudiu.cristea, dawehner, Jaesin, klausi: Add a...
catch’s picture

Version: 8.3.x-dev » 8.2.x-dev

Committed/pushed to 8.3.x.

Leaving RTBC against 8.2.x - tests still running for a start.

claudiu.cristea’s picture

8.2.x test is green too.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 13b6d5f and pushed to 8.2.x. Thanks!

Cheery-picked to 8.2.x cause this makes 8.3.x and 8.2.x test environments the same.

  • alexpott committed 13b6d5f on 8.2.x authored by catch
    Issue #2787923 by claudiu.cristea, dawehner, Jaesin, klausi: Add a...

  • alexpott committed 6be65aa on 8.2.x
    Revert "Issue #2787923 by claudiu.cristea, dawehner, Jaesin, klausi: Add...
alexpott’s picture

Oops there was an 8.2.x specific patch... the modules visibilty how silly. Committed 5d2a732 and pushed to 8.2.x. Thanks!

claudiu.cristea’s picture

Status: Fixed » Reviewed & tested by the community

  • alexpott committed 5d2a732 on 8.2.x
    Issue #2787923 by claudiu.cristea, dawehner, Jaesin, klausi: Add a...

The last submitted patch, 31: 2787923-31-8.2.x.patch, failed testing.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Fixed
claudiu.cristea’s picture

Status: Fixed » Closed (fixed)

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