Refactor the Bazel Test configuration to support running tests on a single file or a single test #2960

DaveShuckerow · 2018-12-13T23:18:59Z

I have a few questions I need to answer and fixes I need to make before submitting this:

How can I test the linecontributor that I added to be sure it will work?
Do we have a system for adding this feature behind a configuration option that users can turn off? I don't want to roll this out and have to roll back if it breaks a few bazel users' workflows.
Refactor common logic from the test configuration producers and line contributors into a common utility so that we have a smaller surface to test.

I would like an initial review and some thoughts on the 3 questions I have left. Thanks!

stevemessick · 2018-12-13T23:35:44Z

resources/META-INF/plugin.xml

@@ -686,7 +686,9 @@
    <programRunner implementation="io.flutter.run.bazel.BazelRunner"/>

    <configurationType implementation="io.flutter.run.bazelTest.FlutterBazelTestConfigurationType"/>
+    <runConfigurationProducer implementation="io.flutter.run.bazelTest.BazelTestConfigProducer"/>


This line will get deleted during the build unless it is added to the template. This may be the cause of the travis failure.

Yup, you'll want to contribute the change to resources/META-INF/plugin_template.xml, and then run ./bin/plugin generate.

devoncarew · 2018-12-14T17:47:36Z

Do we have a system for adding this feature behind a configuration option that users can turn off? I don't want to roll this out and have to roll back if it breaks a few bazel users' workflows.

We do have an 'experimental' section in our preferences dialog; those normally start out as opt-in. You can see the options from the FlutterSettings class.

devoncarew · 2018-12-14T17:53:59Z

DBC: can you attach a screen shot for the UI changes? (FlutterBazelTestConfigurationEditorForm.form)

jwren · 2018-12-14T18:32:36Z

src/io/flutter/run/bazelTest/BazelTestConfigProducer.java

+    return true;
+  }
+
+  private VirtualFile verifyFlutterTestFile(BazelTestConfig config, ConfigurationContext context, DartFile file) {


Possible follow up: @Nullable and @NotNull propagation in IntelliJ can help prevents NPEs

jwren · 2018-12-14T18:36:33Z

src/io/flutter/run/bazelTest/BazelTestConfigUtils.java

+  public static final String WIDGET_TEST_FUNCTION = "testWidgets";
+
+  public static boolean isFlutterTest(DartFile file) {
+    return true;


Should there be a TODO in this method??? If not, perhaps a comment as to why.

This wasn't complete; I've gone back and finished this code. PTAL.

For this section, it was definitely worth fixing instead of leaving it as a TODO.

jwren · 2018-12-14T18:40:10Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.java

-    fields.setBazelTarget(StringUtil.nullize(myBuildTarget.getText().trim(), true));
-    fields.setLaunchingScript(StringUtil.nullize(FileUtil.toSystemIndependentName(myLaunchingScript.getText().trim()), true));
+    final String testName = StringUtil.nullize(this.myTestName.getText().trim(), true);
+    final String entryFile = StringUtil.nullize(FileUtil.toSystemIndependentName(myEntryFile.getText().trim()), true);


Good call wrapping this with FileUtil.toSystemIndependentName(), when we have Windows behaving differently is is typically because I forgot to wrap with this method.

jwren · 2018-12-14T18:41:57Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.java

-    fields.setEntryFile(StringUtil.nullize(FileUtil.toSystemIndependentName(myEntryFile.getText().trim()), true));
-    fields.setBazelTarget(StringUtil.nullize(myBuildTarget.getText().trim(), true));
-    fields.setLaunchingScript(StringUtil.nullize(FileUtil.toSystemIndependentName(myLaunchingScript.getText().trim()), true));
+    final String testName = StringUtil.nullize(this.myTestName.getText().trim(), true);


Shouldn't the nullize be called on this.myTestName, if myTestName is null, trim would throw a NPE.

No. Nullize is the opposite of notNullize: it makes empty strings null.

The text fields, so far as I understand, have @NotNull text fields.

jwren · 2018-12-14T18:43:10Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.java

-    fields.setLaunchingScript(StringUtil.nullize(FileUtil.toSystemIndependentName(myLaunchingScript.getText().trim()), true));
+    final String testName = StringUtil.nullize(this.myTestName.getText().trim(), true);
+    final String entryFile = StringUtil.nullize(FileUtil.toSystemIndependentName(myEntryFile.getText().trim()), true);
+    final String bazelTarget = StringUtil.nullize(myBuildTarget.getText().trim(), true);


Again, shouldn't nullize() be called on the next before trim() is called?

Same.

I've refactored this out into a helper method so that the behavior is easier to keep consistent.

jwren · 2018-12-14T18:43:27Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.java

+    }
+    else if (next != Scope.TARGET_PATTERN) {
+      if (myEntryFile.getText().isEmpty()) {
+        myEntryFile.setText(myBuildTarget.getText().replace("//", ""));


Do you need a call to StringUtil.nullize() like you have in line 84?

Not necessary, but it's a good idea.

This if statement should semantically mean that the text in myEntryFile will be the same as passing a null value. To achieve this, it's best to use the same logic for determining its value as we were using in the other line.

jwren · 2018-12-14T18:44:24Z

src/io/flutter/run/test/TestConfigUtils.java

@@ -34,7 +34,7 @@
  @Nullable
  public static TestType asTestCall(@NotNull PsiElement element) {
    final DartFile file = FlutterUtils.getDartFile(element);
-    if (FlutterUtils.isInTestDir(file) && FlutterUtils.isInFlutterProject(element)) {
+    if (FlutterUtils.isInTestDir(file)/* && FlutterUtils.isInFlutterProject(element)*/) {


Remove commented out code

jwren · 2018-12-14T19:13:07Z

@DaveShuckerow LGTM

On your questions:

line contributor -- lots of manual testing. Even if it was easy and we had bunch of tests for run configurations, lots of manual testing is required here.
System to have work behind a flag-- Devon's experimental comment counts only for things behind that flag/ setting. Since this PR adds to the plugin XML, realize that the IntelliJ framework will come along and start to instantiate the appropriate objects. If you do use the experimental flag make sure that you disable everything you intend to. Again-- lots of manual testing.
Refactoring task. If it is simply a nice refactoring, perhaps follow up with a purely refactoring PR later?

…est commands

…, it always counts as the script being disabled

DaveShuckerow

I added in tests for the command line calls getting generated by the flutter test script; this will help us prevent regressions against the g3 tooling it invokes.

These changes reflect answers to my three questions from the beginning of the PR.

Tests for the linecontributor are manual for now 😢
I added in an experiment to turn on the new bazel test runner.
Pub and bazel logic is different enough that I feel more comfortable separating the utilities than sharing them. I did refactor out some common logic, like the run configuration serialization.

DaveShuckerow · 2019-01-09T22:29:56Z

resources/META-INF/plugin.xml

@@ -686,7 +686,9 @@
    <programRunner implementation="io.flutter.run.bazel.BazelRunner"/>

    <configurationType implementation="io.flutter.run.bazelTest.FlutterBazelTestConfigurationType"/>
+    <runConfigurationProducer implementation="io.flutter.run.bazelTest.BazelTestConfigProducer"/>


DaveShuckerow · 2019-01-09T22:30:48Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.java

-    fields.setEntryFile(StringUtil.nullize(FileUtil.toSystemIndependentName(myEntryFile.getText().trim()), true));
-    fields.setBazelTarget(StringUtil.nullize(myBuildTarget.getText().trim(), true));
-    fields.setLaunchingScript(StringUtil.nullize(FileUtil.toSystemIndependentName(myLaunchingScript.getText().trim()), true));
+    final String testName = StringUtil.nullize(this.myTestName.getText().trim(), true);


No. Nullize is the opposite of notNullize: it makes empty strings null.

The text fields, so far as I understand, have @NotNull text fields.

DaveShuckerow · 2019-01-09T22:31:50Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.java

-    fields.setLaunchingScript(StringUtil.nullize(FileUtil.toSystemIndependentName(myLaunchingScript.getText().trim()), true));
+    final String testName = StringUtil.nullize(this.myTestName.getText().trim(), true);
+    final String entryFile = StringUtil.nullize(FileUtil.toSystemIndependentName(myEntryFile.getText().trim()), true);
+    final String bazelTarget = StringUtil.nullize(myBuildTarget.getText().trim(), true);


Same.

I've refactored this out into a helper method so that the behavior is easier to keep consistent.

DaveShuckerow · 2019-01-09T22:33:06Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.java

+    }
+    else if (next != Scope.TARGET_PATTERN) {
+      if (myEntryFile.getText().isEmpty()) {
+        myEntryFile.setText(myBuildTarget.getText().replace("//", ""));


Not necessary, but it's a good idea.

This if statement should semantically mean that the text in myEntryFile will be the same as passing a null value. To achieve this, it's best to use the same logic for determining its value as we were using in the other line.

DaveShuckerow · 2019-01-09T22:35:59Z

src/io/flutter/run/test/TestConfigUtils.java

@@ -34,7 +34,7 @@
  @Nullable
  public static TestType asTestCall(@NotNull PsiElement element) {
    final DartFile file = FlutterUtils.getDartFile(element);
-    if (FlutterUtils.isInTestDir(file) && FlutterUtils.isInFlutterProject(element)) {
+    if (FlutterUtils.isInTestDir(file)/* && FlutterUtils.isInFlutterProject(element)*/) {


DaveShuckerow · 2019-01-09T22:37:33Z

src/io/flutter/run/bazelTest/BazelTestConfigUtils.java

+  public static final String WIDGET_TEST_FUNCTION = "testWidgets";
+
+  public static boolean isFlutterTest(DartFile file) {
+    return true;


This wasn't complete; I've gone back and finished this code. PTAL.

For this section, it was definitely worth fixing instead of leaving it as a TODO.

devoncarew

@DaveShuckerow, I made a bunch of drive-by comments.

devoncarew · 2019-01-14T23:40:12Z

src/io/flutter/bazel/PluginConfig.java

+    @Nullable String launchScript,
+    @Nullable String testScript
+  ) {
+    final Fields fields = new Fields();


Should we add an additional ctor to Fields here? It would mean less assignment below.

devoncarew · 2019-01-14T23:45:31Z

src/io/flutter/run/bazelTest/BazelTestConfigUtils.java

+import java.util.Arrays;
+import java.util.List;
+
+public class BazelTestConfigUtils {


@DaveShuckerow, how much of this code is from TestConfigUtils? Can we share more here?

A lot is shared. I've refactored these to have a common abstract superclass.

devoncarew · 2019-01-14T23:46:15Z

src/io/flutter/run/bazelTest/BazelTestFields.java

+  }
+
+  // The value to use for the bazel test runner setting if no FlutterSettings are available.
+  // TODO: set up a FlutterSettings implementation that we can use in tests.


We should use TODO(username): here, so people will know who to ask for context.

src/io/flutter/run/bazelTest/BazelTestRunner.java

devoncarew · 2019-01-14T23:49:16Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.form

@@ -1,94 +1,119 @@
 <?xml version="1.0" encoding="UTF-8"?>


Can you attach a screen shot for this UI?

I shared a screenshot for this internally.

devoncarew · 2019-01-14T23:51:37Z

src/io/flutter/run/bazelTest/FlutterBazelTestLineMarkerContributor.java

+import java.util.Map;
+
+
+public class FlutterBazelTestLineMarkerContributor extends RunLineMarkerContributor {


How much of this can we share with FlutterTestLineMarkerContributor?

All of it 😄

src/io/flutter/sdk/FlutterSettingsConfigurable.form

src/io/flutter/settings/FlutterSettings.java

jwren

LGTM

jwren · 2019-01-15T19:04:50Z

src/io/flutter/run/bazelTest/BazelTestFields.java

+    // The order we do the checks here determines priority.
+
+    final DartSdk sdk = DartPlugin.getDartSdk(project);
+    if (sdk == null) {


Do you need a call to sdk.getVersion().flutterTestSupportsFiltering(); here as well, like above?

No, and for two reasons:

bazel test filtering is managed separately from the Flutter SDK AFAIK. Test filtering support was added at a later change in g3 than when it was added to the Flutter SDK.

when the test runner is not available, all test configurations will use the legacy behavior, which doesn't attempt to filter targets.

src/io/flutter/run/bazelTest/BazelTestFields.java

jwren · 2019-01-15T19:11:56Z

src/io/flutter/run/bazelTest/FlutterBazelTestLineMarkerContributor.java

+      final int lineNumber = document.getLineNumber(textOffset);
+
+      // e.g., dart_location:///Users/pq/IdeaProjects/untitled1298891289891/test/unit_test.dart,3,2,["my first unit test"]
+      final String path = containingFile.getVirtualFile().getPath();


Paths should be processed through FileUtil.toSystemIndependentName(). If such a call breaks some string assumption here, document the reason.

jwren · 2019-01-15T19:13:06Z

src/io/flutter/utils/ElementIO.java

@@ -0,0 +1,41 @@
+/*
+ * Copyright 2018 The Chromium Authors. All rights reserved.


nit: copyrights can be 2019 now

Update all files or just newly added ones?

From discussion, it looks like we leave copyrights on files as the year the file was created, so new files will be copyright 2019, files I created in 2018 will be copyright 2018, and other files should have their copyrights untouched.

…ular and bazel-flavored tests

DaveShuckerow · 2019-01-16T21:21:16Z

We need to leave a warning when the bazel test runner experiment is turned on and the test runner is not available so that users can sync their clients.

… be found

DaveShuckerow

I believe I've responded to all the comments.

Please take another look.

DaveShuckerow · 2019-01-16T21:46:34Z

src/io/flutter/run/bazelTest/BazelTestFields.java

+  }
+
+  // The value to use for the bazel test runner setting if no FlutterSettings are available.
+  // TODO: set up a FlutterSettings implementation that we can use in tests.


DaveShuckerow · 2019-01-16T21:46:56Z

src/io/flutter/run/bazelTest/BazelTestConfigUtils.java

+import java.util.Arrays;
+import java.util.List;
+
+public class BazelTestConfigUtils {


A lot is shared. I've refactored these to have a common abstract superclass.

DaveShuckerow · 2019-01-16T21:52:47Z

src/io/flutter/run/bazelTest/BazelTestConfigProducer.java

+    return true;
+  }
+
+  private VirtualFile verifyFlutterTestFile(BazelTestConfig config, ConfigurationContext context, DartFile file) {


DaveShuckerow · 2019-01-16T22:40:57Z

src/io/flutter/run/bazelTest/FlutterBazelTestLineMarkerContributor.java

+import java.util.Map;
+
+
+public class FlutterBazelTestLineMarkerContributor extends RunLineMarkerContributor {


All of it 😄

DaveShuckerow · 2019-01-16T22:41:39Z

src/io/flutter/run/bazelTest/FlutterBazelTestLineMarkerContributor.java

+      final int lineNumber = document.getLineNumber(textOffset);
+
+      // e.g., dart_location:///Users/pq/IdeaProjects/untitled1298891289891/test/unit_test.dart,3,2,["my first unit test"]
+      final String path = containingFile.getVirtualFile().getPath();


src/io/flutter/run/bazelTest/BazelTestFields.java

src/io/flutter/run/bazelTest/BazelTestRunner.java

DaveShuckerow · 2019-01-16T23:13:19Z

src/io/flutter/run/bazelTest/FlutterBazelTestConfigurationEditorForm.form

@@ -1,94 +1,119 @@
 <?xml version="1.0" encoding="UTF-8"?>


I shared a screenshot for this internally.

src/io/flutter/sdk/FlutterSettingsConfigurable.form

src/io/flutter/settings/FlutterSettings.java

…to bazel-test

Dave Shuckerow added 5 commits November 26, 2018 14:14

Work on the contributor

84220ea

Make the bazel test runner work with individual files

495dea6

Clean up test fields and configuration form

7ee2dbf

Remove bazel test launcher script from the configuration; no longer used

f79556a

Context menus work

5c482e6

googlebot added the cla: yes label Dec 13, 2018

DaveShuckerow requested a review from jwren December 13, 2018 23:19

Clean up template.xml and configutils, add a fallback to the test fields

5880036

stevemessick reviewed Dec 13, 2018

View reviewed changes

jwren approved these changes Dec 14, 2018

View reviewed changes

Dave Shuckerow added 6 commits December 21, 2018 13:15

Remove some test references from the bazelTest code.

000b0c7

Get the test config producer working and set up a test file for the t…

1cd0a63

…est commands

Merge remote-tracking branch 'upstream/master' into bazel-test

30cc41a

Add tests for the test config

2c828ec

Add a setting for the new bazel test runner

2a5de05

Simplify the settings so that when the tester script is not available…

a393164

…, it always counts as the script being disabled

DaveShuckerow commented Jan 9, 2019

View reviewed changes

Dave Shuckerow added 5 commits January 9, 2019 15:51

Make the config producer work correctly

bb09ce6

Remove redundant word

7fbdfc7

Bazel serialization test

8c88e29

Tests for serialization

3ce5b53

File independence for the test fields

6d14464

devoncarew reviewed Jan 14, 2019

View reviewed changes

jwren approved these changes Jan 15, 2019

View reviewed changes

Start refactoring test config utils to share common logic between reg…

a953921

…ular and bazel-flavored tests

Nullables and fields refactoring

a375ac2

Dave Shuckerow added 6 commits January 16, 2019 14:18

Remove unused directory config producer

03da37d

Simplify line marker contributors to have simpler dependencies

93272cd

Extra whitespace

934cf3e

Update settings so that a warning shows when the new test runner cant…

dc98dd6

… be found

Revert testrunner changes

7abf1cf

Remove dead code

84e6a67

DaveShuckerow commented Jan 16, 2019

View reviewed changes

Dave Shuckerow added 2 commits January 17, 2019 12:42

Adjust override for bazel test fields setting

c223926

Merge branch 'master' of ssh://github.com/flutter/flutter-intellij in…

29aec71

…to bazel-test

DaveShuckerow merged commit c307e0e into flutter:master Jan 17, 2019

DaveShuckerow deleted the bazel-test branch February 4, 2019 23:15

		import java.util.Map;


		public class FlutterBazelTestLineMarkerContributor extends RunLineMarkerContributor {

		@@ -0,0 +1,41 @@
		/*
		* Copyright 2018 The Chromium Authors. All rights reserved.

Refactor the Bazel Test configuration to support running tests on a single file or a single test #2960

Refactor the Bazel Test configuration to support running tests on a single file or a single test #2960

Conversation

DaveShuckerow commented Dec 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew commented Dec 14, 2018

devoncarew commented Dec 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwren commented Dec 14, 2018

DaveShuckerow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveShuckerow commented Jan 16, 2019

DaveShuckerow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveShuckerow commented Dec 13, 2018 •

edited

devoncarew commented Dec 14, 2018 •

edited