Prepare Tests for upgrade.

I hope we can work together, in a community minded way, to bridge the clear differences in our positions
and produce good quality test that we are both happy with...

I take the line that the discussion is the most important thing and the code that follows is just the by-product.

Let me start by criticising myself -- my argument about hiding test on a production
server is poor if an attacker knows the module is open source!

Perhaps looking at some solid code will save 1000 words... I was mostly complete..
now I am 95% and I still think the idea has merit.

I am including a screen shot ( taken with a call to $this->createScreenshot() on the last page )

It shows the article page, with our 3 distinct maps on display - everything about to pass.

I believe strongly in posting early, post often .... so please bear in mind the test are broken

I gave it up (the code ended up as a bunch of terrible hacks).

Perhaps what I done will only confirm to your mind that it cannot be done cleanly.

I am involved in those core conversion issues and followed the discussion/development
of common code patterns closely...

I disagree that we should wait ... I will try and summarise recent goings-on and problems with
FunctionalJavascrtipt .. please jump in if you have an alternate perspective.

There WERE lots of unrepeatable tests which unfortunately passed enough to go through final committers review and that caused lots of reverts
- a major headache.

but the culprit was found, it was

$this->assertSession()->assertWaitOnAjaxRequest()

the method was not safe .. in any event the best working pattern has now been established.

"Wait for the page DOM to reach a final state that demonstrates the request/response cycle is complete before
continuing with assertions."

I am so used to following this pattern that converting this test was easy -
I would say "Thing slotted together nicely."

Returning to unresolved core FunctionalJavascript issues -- we do not have a good pattern of injecting
malicious data and asserting that that a http 500 error code is generated - that and the whole nighwatch thing but those issues do not factor here.

To aid review here is a crib sheet about deprecated functions.

1) $this->assertText() - mostly I used deliberately over-specified css selectors to provide additional assertions about what I was expecting
see calls to ->find('css', 'some tighly constrained css selector')

2) $this->assertLink() was replaced by $this->assertSession()->linkExists()

I appreciate reviewing things takes time ... so in the main I opted for a straight conversion where possible
here are some observations I have, which I did not change, but should go in a later issue/refactor

1) The test do some unnecesaary setup through the UI and doing things programatically will be faster.

Specifically it is good design to limit the UI manipulation to only the class under test
( our formatter and it settings form ) we have a few calls to 'admin/structure/types/manage/article/fields/add-field' at the top of the test which should be simplified.

2) The last few tests involve searching for an image with src that contains google.com/map we should be more precise and check all three images

3) I started this issue because I was thinking about writing test for #2896382: Support for Geolocation field. .. augmenting our test with an additional field seems to way to proceed there --- and that seems a natural fit for a FucntionalJavascript test.

I have tried speaking neutrally, laying out my argument ... I hope this provides a good framework for constructive criticism. Please feel free to tear anything apart.

Also If I can answer any questions in general about FunctionalJavascript testing please let me know.

I will continue to fix the final few lines of the test.

sorry here is the patch

There is a difference between what I see locally and what testbot reports .. but before I debug that I think I want to short circuit the whole thing...

I am about to propose a major redesign of the tests

Overnight I thought more about my unease in relation to configuration setup through the UI of field settings..not directly related to the class under test.

When I pushed that notion to its extreme this is what I came up with

"Use a dedicated test module to define a new content type containing three pre-configured fields
with default settings and values which map to our fields map1, map2 and xss"

The test then can be downgraded to a function level test ( extending BrowserTestBase )

By loading the test module and creating a 'simple_gmap_stress_test' node we in effect jump immediately to the final page - where we can start making relevant assertions.

Again a code snippet might save a thousand word... so instead of throwing away the existing changes I have, in parallel, created an outline of the test-submoule and the new test. It is a skeleton level piece of code where I have yet to specify the final assertions

To speak positively about this idea...

Maintenance is easier if we need to reasons with test setup we can copy the test module into module/custom and "drush en simple_gmap_stress_test" and visualize what is going on in a browser.

To speak negatively....

There is a small amount of conditional logic in SimpleGMapFormatter::settingsSummary() for example

    if ($include_static_map) {
      $summary[] = $this->t('Static map: @width x @height, Scale: @static_scale', [
        '@width' => $this->getSetting('iframe_width'),
        '@height' => $this->getSetting('iframe_height'),
        '@static_scale' => $this->getSetting('static_scale'),
      ]);
    }

and if we wanted to test it we would have to go back to javascript tests.

Anyway posting early ... posting often it is not yet worth moving to need review.

The test then can be downgraded to a function level test ( extending BrowserTestBase )

Ah, I see I am unexpectedly the initiator or false rumours

"functionAL level tests" --- FunctionalTests are a little less complex to setup compared to FunctionalJavascriptTests

I did not mean to start the unit test rumour.... -:)... Anyway .. I have yo-yo'ed back to FunctionalJavascript Testing

When I spoke negatively in #10 .. I was talking out loud ... feeling my way forward

This adjustment, is my way of addressing both our concerns.

testSettingsForm - stops any regression blunders with regard to the settings forms. The thought behind it is - "let add a blank field which is not not required and then as needed we can make assertions to confirm we have driven each form values away from the default."

testFormatterOutput - addresses my drive for clarity - get in -- test something specific --- then get out.
.. it is only 25 line long.

compared to my last iteration the test class line count is reduced by 40% and executes 15% faster....
numbers not worth having... only as indication of a cleaner expression the test strategy.

Again this is skeleton code... while I think out loud ...and get the broad outline correct.. I am reading some core documents describing how we can use ->createSceenshot() to create assets which remain in the test record ... so we can do more of the manual steps automatically ... as this issue closes I hope to open a new issue so we can fully automate the remaining tests.

I will not be able to finish this until Sunday.. I just to see if I have resolved the difference between what I see locally and what testbot reports.

Found a little time to work on this

a) fixed some coding standard nits
b) test should be run with the --browser option for a visual check. I have removed the hardcoded screenshot call that I was using for debug.
c) Replace flag as a group identifier.
d) Removed stray dependencies .. in the entwined config definitions - so no more unmet dependency warnings.

test still do not pass .. and I have not addressed all the review comments above but ... progress.

Cherry picking the obvious flaw.

@jhodgdon thanks for nudging me on this .. I did drop the ball..

1) Regarding the simple_gmap_stress_test module and the content type it defines.

a) In response to a deprecation notice I have added a match_limit entry to the uid settings type

Here is the change record https://www.drupal.org/node/3024684

b) One of the reasons the XSS test was failing was that the include_text was set to false.

2) mink is particular about the CSS selectors it accepts ... so this solves the next error.

-    $blank_row = $fields_form->find('css', "#field-blank");
+    $blank_row = $fields_form->find('css', "tr#field-blank");

3) setting a default theme is now needed to suppress deprecation notices.

+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';

4) I deleted tests/src/Functional/StressTest.php as it was not really testing much...

5) From core I begged borrowed and stole the test for XSS from the menu router test

Drupal\Tests\system\Functional\Menu\MenuRouterTest::doTestHookMenuIntegration()

so

+$this->assertEscaped("<script>alert(\"hello\");</script>", ENT_QUOTES, 'UTF-8');

I am posting early .. but I wanted to show progress .. I think I need another night to sit on this and think about it.

This passes locally for me .. famous last words.

Out of paranoid I did a fresh site-install on my test environment. No joy.
I can't see anything immediately obvious in the build/test artefacts.

It maybe the weekend before I can look at this more closely.

Thanks I agree, assertWaitOnAjaxRequest(); is a possible solution, and certainly points to what I think is the underlying problem.

In relation to this article

https://www.drupal.org/docs/8/phpunit/phpunit-javascript-test-writing-tu...

assertWaitOnAjaxRequest() is listed as a way forward... However I did alot of conversions of FunctionalJavascript tests in core .. and I can say I got pushback during review.

As I understand it assertWaitOnAjaxRequest() remains as the solution of last resort and solutions of the form

::waitForElement()
::waitForElementVisible()
::waitForButton()
::waitForLink()
::waitForField()
::waitForId()

are preferred.

Next I want also to talk about another aspect of the test design I am unhappy with

when I run

./vendor/bin/phpunit -c core --verbose modules/contrib/simple_gmap

where --verbose does the same thing as --debug

debug/verbose .. outputs the visited pages to a browser ... and I can see the tests servers have reached out to google and the tests pull in the relevant images as they run.

I think this is bad, specifically my bad ... and I am also looking for ways of mocking request for google as we don't inspect the received images anyways.

I have converted the tests with this signature

/**
   * Inspect the formatter output.
   *
   * Find one good example and two troublesome senarios.
   *
   * Troublesome senarious :-
   *   A complex character set.
   *   A XSS attack.
   */

into a Kernel test ... Sorry for the flip flopping...

In conversational language the new tests algorithm :-

Pass troublesome simple_gmap fields through the pipeline which is the 'view builder' and 'renderer'
and make assertions about the computed text.

To speak up for this approach ... the test infrastructure no longer inadvertently reaches out to google with requests, and Kernel test run much faster that Functional tests ( about 2 seconds on my machine )

One aspect of this approach I am in two minds about :-

Developers new to this module may have to install the stress_test module, inspect node/1 before they get understanding of what is going on .. some '*FormatterTests.php' files in core build up a field under test in a more accessible fashion.

Anyway this is the next iteration.

Jhodgdon thanks for following along.

(a) Something I noticed while scanning the sister issue.

#3088320: Make sure Simple Google Maps is fully Drupal 9 compatible

I am modifying the info file to pass the installation compatibility test that come with Drupal9

(b) I have fixed up some coding standard issue I introduced.

(c) I think I have resolved the test-instability by inserting a interlock ..

we now wait for the form to appear before manipulating the html inputs

A) I am simplifying the test code without any expectation of solving the underlying problem.

Previously the code could be read as

1) Find the form element.
2) Within the form find the row associated with the 'blank' field.
3) Within the row find the button
4) Click the button.

I have simplified the code to be

1) Find the button
2) click the button.

B) The test failure associated with the Drupal9 run fails for different reasons. I have fixed that.

At the moment I guess the failure is timing related or at least I have insufficiently locked things down.
Just in case it is not that I am documenting some version numbers in my local test setup

php 7.3.12
phpunit 7.5.18

Ok .. that is unexpected ... the solution to nebulous questions like "why does it work only on my machine" are seldom straightforward.

The best I have is "well the solution has less moving parts".

Maybe the page and the form arrive in chunks and waiting for the button is the most robust solution.

If my solution is not complete... it will cause problems down the line...

As a side note

The test originally put Effiel tower in the form and checked for the output text 'Effiel tower'.

I removed it because it was not adding anything to the coverage. I did not adjust a related comment ... I am doing so now.

I think this is ready now.

I looked this over tonight and I think this is good to go ...

It has been nearly a year since I have commited anything and my RSA keys are askew..

I while fix up that up in the morning over breakfast and then push

jhodgdon thanks for your comments -- they shaped the direction of this fix.