Provide the ability for JS functional tests to track whether the page has been reloaded

Nice functionality and really nice solution!

We could add this asssertion to our WebAssert class if we store the state non on the class but rather locally. Do you think the DX would be worse as a result of that?

Yeah ... what is your thought?

I like the current way rather than returning the string. I feel like it would be easier to make a mistake. In the current way we could even throw an exception if $pageReloadMarker was never set and 1 of the other 2 functions is called.

Also the way it is not we are actually free to change the way it actually works to mark the page later.

You could jQuery.once that element. if `jQuery.once === 1`, that's no changes.

** It should be equal to ONE. Two or more and ZERO, the test env is polluted.

Wouldn't this require jquery once on every page? I think having no extra dependency would be nice.

@dawehner

For Contrib way yes, otherwise it always loaded with any ajax in D8 Core.

From the big picture, it's the best I think.

DOM Manipulation has side effect and jQuery.once is a standard thing in Drupal, every script could exclude it globally :)

About the current patch, I suggest adding an ID to the new DOM and let other scripts could exclude it from/trigger a re-render. (these kinds of scripts: https://www.drupal.org/node/2645250)

(it's also best to use a standard prefix. Probably it should add .hidden to minimize any side effect too)

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -139,6 +146,34 @@ protected function createScreenshot($filename, $set_background_color = TRUE) {
+    $this->getSession()->executeScript('document.body.appendChild(document.createTextNode("' . $this->pageReloadMarker . '"));');

Could we store actually this machine name somewhere on the window ... object maybe?

@dawehner - Nice point. I think if we can avoid DOM manipulation all together for this test we'll be better off; maybe window.Drupal.pageReloadMarker ?

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -139,6 +139,55 @@ protected function createScreenshot($filename, $set_background_color = TRUE) {
+      (function() {
+        window.Drupal = window.Drupal || {};
+        return window.Drupal.testPageReloadMarker;
+      })();

Minor note, evaluateScript requires multiline scripts to be wrapped in an IIFE, let's maybe document that?

can we replace EOS to JS? Some editors has JS highlighting :)

This is a much nicer solution now!!!

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Tests/JavascriptTestBaseTest.php
@@ -0,0 +1,36 @@
+  /**
+   * @covers ::markCurrentPage
+   * @covers ::assertPageNotReloaded
+   */
+  public function testAssertPageNotReloaded() {
+    $this->drupalGet('');
+    $this->markCurrentPage();
+    $this->assertPageNotReloaded();
+  }
+

I think for an actual test coverage it would be nice to have some interactive behaviour going on in the meantime

re #19 I am not sure we should add interactive behavior to the testAssertPageNotReloaded() we are testing the assertPageNotReloaded not whether any particular activity does or doesn't reload the page.

Maybe as follow up we could look at adding this to current tests that testing our ajax system but I think it is pretty untested with functional javascript tests.

Maybe executing some HTTP request using jQuery.Get to the frontpage or so. It just feels like doing nothing also doesn't really cover it properly.

I was the pain in the ass, so I should also add the test coverage ... I'm not sure these tests pass, I don't have time right now :(

I forgot to add some files ...
This time the tests though also pass.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Tests/WebDriverTestBaseTest.php
   @@ -0,0 +1,45 @@
   +  public function testAssertPageReloaded() {
   ...
   +  public function testAssertPageNotReloaded() {
   

   For both of these test methods should have test that assertion actually fails if the page is/isn't reloaded.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
   @@ -169,6 +169,55 @@ protected function createScreenshot($filename, $set_background_color = TRUE) {
   +  /**
   +   * Marks the page to assist determining if the page has been reloaded.
   +   */
   +  protected function markCurrentPage() {
   

   I think this should have @see to getCurrentPageMarker()
   also a @see in the other direction.

3. So the pattern for using for these test methods would be
   1. You call markCurrentPage() to mark the page
   2. You either call assertPageNotReloaded() or assertPageReloaded()
   3. in either case getCurrentPageMarker() get called.

   Would it be logical error or a problem in your test if you call markCurrentPage() 2x without calling one of the asserts above?
   We could maybe avoid some bad tests but actually checking getCurrentPageMarker() inside to see if it is already marked and throw an exception if it is.
   Maybe markCurrentPage() could have parameter $clear_mark which would default to FALSE but if TRUE would not throw the exception(and not check internally)

   I just don't know of another case where we have these to interelated test methods and might want help with this because easy to make logic errors in tests and not know what you are actually testing.

I played around with this some more.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
   @@ -169,6 +169,55 @@ protected function createScreenshot($filename, $set_background_color = TRUE) {
   +    $this->assertNotEmpty($this->getCurrentPageMarker());
   

   Both of the new assertions don't have a custom message so get an error message like
   "Failed asserting that a string is empty."

   This is not very helpful. Could we change these to custom messages about the reload state.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
   @@ -169,6 +169,55 @@ protected function createScreenshot($filename, $set_background_color = TRUE) {
   +  protected function assertPageReloaded() {
   +    $this->assertEmpty($this->getCurrentPageMarker());
   

   If at test calls assertPageReloaded() without calling markCurrentPage() first the assert actually just passes. And we don't have a @see for markCurrentPage() so it seems reasonable this would happen. Even with the @see

   We could just add the @see but I think it would be better to make sure that does happen.
   We could add WebDriverTestBase::$pageMarkedCalled which would default to FALSE and be set to TRUE at the end of markCurrentPage(). Then in getCurrentPageMarker() which both the new assertions use we could throw an exception or $this->fail() with a message about calling markCurrentPage() if $pageMarkedCalled is still FALSE.