Create a Views and ViewsUI FunctionalJavascriptTestBase class

Nice! Mostly nitpicks:

1. +++ b/core/modules/views/tests/src/FunctionalJavascript/ViewTestBase.php
   @@ -0,0 +1,84 @@
   + * @see \Drupal\simpletest\WebTestBase
   

   No need to reference this right?

2. +++ b/core/modules/views/tests/src/FunctionalJavascript/ViewTestBase.php
   @@ -0,0 +1,84 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   can just be an {@inheritdoc} right?

3. +++ b/core/modules/views/tests/src/FunctionalJavascript/ViewTestBase.php
   @@ -0,0 +1,84 @@
   +  /**
   +   * @param bool $import_test_views
   +   */
   +  protected function setUp($import_test_views = TRUE) {
   

   Not quite a proper docblock

4. +++ b/core/modules/views/tests/src/FunctionalJavascript/ViewTestBase.php
   @@ -0,0 +1,84 @@
   +  protected function enableViewsTestModule() {
   ...
   +  protected function schemaDefinition() {
   ...
   +  protected function viewsData() {
   ...
   +  protected function dataSet() {
   

   Can we/should we move these to a trait?

5. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/UITestBase.php
   @@ -0,0 +1,52 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   can just be an {@inheritdoc} right?

6. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/UITestBase.php
   @@ -0,0 +1,52 @@
   +  protected function setUp($import_test_views = TRUE) {
   

   Do we want to add the disabling of the 'auto-preview' here too? It gets disabled in most existing Views tests and could get messy when not disabled.

Starting to look really good! Should we maybe move all the existing javascript tests over to the new test base and use them as a test case for the new base class instead of the preview test? (nice to see that green though!).

1. +++ b/core/modules/views/src/Tests/ViewCreationTrait.php
   @@ -0,0 +1,60 @@
   +trait ViewCreationTrait {
   

   ViewTestDataSetupTrait or just ViewTestDataTrait maybe? I think the location is fine, that's where all the other traits live for now (but others might have other ideas?)

2. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/UITestBase.php
   @@ -0,0 +1,70 @@
   +  public static $modules = ['node', 'views', 'views_test_config', 'views_ui'];
   

   Any way to leave 'node' out of this? Really don't want to always have that enabled.

@michielnugter sweet! I really love all the red in that patch, it's really cleaning this up nicely!

Should I open up a separate issue or can I fix it here?

Lets put both those points in follow ups, this is getting big enough as it is.

1. +++ b/core/modules/views/tests/src/FunctionalJavascript/ViewTestBase.php
   @@ -0,0 +1,62 @@
   +  public static $modules = ['node', 'views', 'views_test_config'];
   

   Is 'node' really needed for every Views test? That would really be a shame. I'd really be for removing it here and just adding public static $modules = ['node']; to the tests that do need it.

2. +++ b/core/modules/views/tests/src/FunctionalJavascript/ViewTestBase.php
   @@ -0,0 +1,62 @@
   +  protected function setUp($import_test_views = TRUE, $create_user = TRUE) {
   
   +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/UITestBase.php
   @@ -0,0 +1,63 @@
   +  protected function setUp($import_test_views = TRUE, $enable_test_views_module = TRUE, $disable_auto_preview = TRUE, $create_user = TRUE) {
   

   Instead of putting a lot of parameters on the setup method wouldn't it be better to just use class properties like you did for protected $adminUserPermissions? I think that makes it much cleaner (and less annoying to override setup())

+++ b/core/modules/views/tests/src/FunctionalJavascript/ViewTestBase.php
@@ -0,0 +1,83 @@
+    if (self::$createUser) {

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/UITestBase.php
@@ -0,0 +1,66 @@
+    if (self::$enableTestViewsModule) {
...
+    if (self::$disableAutoPreview) {

I think this needs to be static:: not self::, otherwise the base value is always used unless you override the method.

Other then that I think this looks great.

Once #2863267: Convert web tests of views lands we should probably also use the ViewTestDataTrait on the BrowserTestBase base class

I really like how this looks and how it cleans up all the existing tests!

Sorry about taking so long to answer, had a bit of a think about this, and discussed it a little bit with @michielnugter offline.

I would have said the base classes are maybe not needed, but the traits are helpful, given its special code.

I'm inclined to agree with this. I do like the amount of duplicate code that the base classes remove, but are they NEEDED, no not really, we manage fine without them, and leaving them out gives us one less thing to maintain. (One of the things I was pondering, was that this is a bit of a deviation from how it was done before, where we had base classes everywhere, even completely empty ones, @see \Drupal\views\Tests\Handler\HandlerTestBase. Not saying that was better, just a different approach).

I agree the traits are really useful though. Great for creating Views tests build on other base classes.

And since #2863267: Convert web tests of views landed, lets use the trait on \Drupal\Tests\views\Functional\ViewTestBase

@michielnugter maybe any more thoughts on this?