Make most ITs work with both linux and windows #143

stephenashank · 2019-09-24T04:06:03Z

This is a proof of concept, see CONTRIBUTING.md for information on the integration tests. I'll be updating the documentation there as well.

Once you have a disk image created for windows, and the corresponding environment variables, you will be able to run mvn -Dit.windows=true verify and run the tests using windows agents. This should increase test coverage and get rid of the need to have separate duplicated tests.
When running simply mvn verify the test behavior is unchanged from before this change.

There are still issues with this PR when it comes to running the windows tests, but I wanted to get this out for early feedback.

stephenashank · 2019-09-24T04:11:50Z

...a/com/google/jenkins/plugins/computeengine/integration/ComputeEngineCloudWorkerFailedIT.java

@@ -63,6 +65,7 @@

  @BeforeClass
  public static void init() throws Exception {
+    Assume.assumeFalse(windows);


Currently, the premise of the ComputeEngineCloudWorkerFailedIT is that worker creation fails because there is no startup script which means Java is not installed on the agent. However, the way that Windows agents are created for integration tests right now means that java comes pre-installed on the image.
Unlike assertFalse, which leads to a failure, assumeFalse will skip tests. We can use this to skip tests it doesn't make sense to run with windows agents like this.

craigdbarber · 2019-09-25T22:31:43Z

LGTM.
Were there meant to be changes made to the contributing docs with this PR?

craigdbarber · 2019-09-25T22:32:27Z

Forgoing clicking the approve button, as I assume you'll want to merge a later PR in a finished state.

stephenashank · 2019-09-26T00:39:37Z

Still having issues with the tests themselves. This will include changes to the documentation when those are working.

…and number of concurrent tests.

…gion.

craigdbarber · 2019-10-14T22:01:52Z

jenkins/Jenkinsfile

+ * limitations under the License.
+ */
+
+// Windows Agent IT pipeline, run manually.


Having two files named "Jenkinsfile" could be confusing. Perhaps we can have Jenkinsfile.windows and Jenkinsfile.linux, both in the top level directory. The CI configuration allows you to specify the filename, so we're not limited to the "Jenkinsfile" naming convention.

craigdbarber · 2019-10-14T22:03:15Z

src/test/java/com/google/jenkins/plugins/computeengine/integration/Commands.java

+import lombok.Getter;
+
+@Getter
+public enum Commands {


Missing javadoc header.

craigdbarber · 2019-10-14T22:04:36Z

...ns/plugins/computeengine/integration/ComputeEngineCloudMultipleMatchingConfigurationsIT.java

@@ -122,7 +119,6 @@ public void testMultipleLabelsProvisionedWithLabels() throws Exception {
  private void checkOneNode(PlannedNode plannedNode, String desc)
      throws InterruptedException, java.util.concurrent.ExecutionException {
    String name = plannedNode.displayName;
-    plannedNode.future.get();


What's the reasoning for removing this part?

The thing that we care about most in this test is whether both configurations were used, which can be tested without waiting for the instances to fully launch by looking at the configuration of each planned node. This also speeds up this test, especially in the windows case.

craigdbarber · 2019-10-14T22:07:25Z

...java/com/google/jenkins/plugins/computeengine/integration/ConfigAsCodeNonStandardJavaIT.java

+import org.junit.rules.Timeout;
+import org.jvnet.hudson.test.JenkinsRule;
+
+public class ConfigAsCodeNonStandardJavaIT {


Missing javadoc header.

craigdbarber · 2019-10-14T22:09:11Z

...java/com/google/jenkins/plugins/computeengine/integration/ConfigAsCodeNonStandardJavaIT.java

+    // Wait for the node creation to finish
+    planned.iterator().next().future.get();
+    Instance instance = client.getInstance(PROJECT_ID, ZONE, name);
+    assertNotNull(instance);


Do we think there's value in attempting to run a command to ensure Java is properly installed? Perhaps "java version", and making sure it doesn't error.

This is tested for in the launch process, which will only complete if it's able to use java to start the agent process. This test shouldn't progress beyond line 80 until that finishes. Maybe it would make sense to have a build step here, which would confirm that the agent jar was used and java is properly installed.

craigdbarber · 2019-10-14T22:12:41Z

src/test/java/com/google/jenkins/plugins/computeengine/integration/ITUtil.java

  static final String SSH_PRIVATE_KEY = SSH_KEY.getPrivateKey();
+  private static final String WINDOWS_STARTUP_SCRIPT =


Would it make sense to de-dup these startup scripts with their counterparts in the launchers? Same question with the SSH setup logic here.

… properly installed. Adjust timing for RestartPreemptedIT

…single connection instead of trying to connect twice.

stephenashank · 2019-10-16T04:02:35Z

src/test/java/com/google/jenkins/plugins/computeengine/integration/Commands.java

+public enum Commands {
+  ECHO("echo %s", "echo %s"),
+  EXIT("exit /b %s", "exit %s"),
+  SLEEP("ping /n %s /l 1024 google.com ", "sleep %s");


Yes, this is a little weird. I was not able to get the proper windows equivalent "timeout" to work. Looking around I found that there are some weird issues with running the command in non-interactive scripts, discussed in places such as this comment.

Makes sense. Is using Powershell a viable alternative to batch? If so, we could consider using -NonInteractive: https://docs.microsoft.com/en-us/powershell/module/Microsoft.PowerShell.Core/About/about_PowerShell_exe?view=powershell-5.1#-noninteractive

Thanks for the tip, while I had trouble using powershell within the existing batch scripts, this led me to finding the Powershell plugin which automatically applies the NonInteractive flag when running scripts, and I was able to remove this hack.

stephenashank · 2019-10-16T04:05:52Z

src/main/java/com/google/jenkins/plugins/computeengine/PreemptedCheckCallable.java

@@ -58,7 +58,18 @@
   */
  @Override
  public Boolean call() throws IOException {
-    HttpRequest request = createMetadataRequest();
+    HttpRequest initialRequest = createMetadataRequest(false);


This came up in #148 as well. Having the preemption event occur before the agent is running and listening for preemption events is rare, but given that it can and does happen it makes sense to check that it's not already preempted before beginning the long poll to wait for a change to the metadata (which won't happen in that case).

Makes sense.

craigdbarber · 2019-10-16T17:42:47Z

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineLinuxLauncher.java

      }
-    } finally {
+    } catch (Exception e) {


In general I'm weary of swallowing exceptions like this. I think at the very least we should consider logging it as a warning.

Added logging

craigdbarber · 2019-10-16T17:43:42Z

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineWindowsLauncher.java

@@ -87,8 +78,7 @@ private boolean authenticateSSH(
    return isAuthenticated;
  }

-  private boolean bootstrap(ComputeEngineComputer computer, TaskListener listener)
-      throws IOException, Exception { // TODO(evanbrown): better exceptions
+  private Optional<Connection> bootstrap(ComputeEngineComputer computer, TaskListener listener) {
    if (computer == null) {


Preconditions.checkNotNull()

craigdbarber · 2019-10-16T17:44:05Z

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineWindowsLauncher.java

      }
-    } finally {
+    } catch (Exception e) {


Same comment as above re: swallowing exceptions.

Added logging

…, use powershell to use sleep command. Update documentation for running windows ITs. Remove obsolete windows IT files.

…rations test more reliable.

craigdbarber · 2019-10-17T18:37:09Z

Wanted to verify these deletes were intentional:

...test/java/com/google/jenkins/plugins/computeengine/integration/ConfigAsCodeWindowsIT.java
...ava/com/google/jenkins/plugins/computeengine/integration/ComputeEngineCloudWindowsIT.java

stephenashank · 2019-10-17T18:51:10Z

Yes, those are no longer needed. ConfigAsCodeTestIT now replaces ConfigAsCodeWindowsIT, while ComputeEngineCloudWorkerCreatedIT, ComputeEngineCloudSnapshotCreatedIT, and ComputeEngineCloudIgnoreProxyIT replace ComputeEngineCloudWindowsIT when running the windows integration tests.

craigdbarber

LGTM

stephenashank added the WIP Work in progress label Sep 24, 2019

stephenashank requested a review from craigdbarber September 24, 2019 04:06

stephenashank commented Sep 24, 2019

View reviewed changes

Stephen Shank added 8 commits October 10, 2019 14:16

Make most ITs work with both linux and windows

e0860ed

Rename misspelled file.

43ae720

Use generic commands which can vary based on the platform.

a608c41

Split ConfigAsCodeTest into two different tests.

60b2464

Adjusting timing through Awaitility and nodefuture.get().

7aaa488

Create independent profile for windows ITs, with different run order …

df8e355

…and number of concurrent tests.

Consolidate SSH keys and users.

5423f9d

Add pipeline script for running windows ITs.

31a07f2

stephenashank force-pushed the feature/issue49windows branch from 641fbe1 to 31a07f2 Compare October 12, 2019 01:03

Stephen Shank added 2 commits October 14, 2019 14:12

Loosen tolerance for SnapshotCreated test. Fix ConfigAsCodeIT zone/re…

98069b1

…gion.

Use service account email for windows config as code it.

7149c5c

craigdbarber reviewed Oct 14, 2019

View reviewed changes

Stephen Shank added 4 commits October 14, 2019 15:41

PR feedback: Add class header javadoc, add build step to confirm java…

5fbf016

… properly installed. Adjust timing for RestartPreemptedIT

PR Feedback: Change Jenkinsfile names. Adjust profile and timeouts.

70e6a1f

Fixes for bugs uncovered by test unreliability. In particular, use a …

6bb42b9

…single connection instead of trying to connect twice.

Cleanup

fee03f9

stephenashank commented Oct 16, 2019

View reviewed changes

stephenashank changed the title ~~WIP: Make most ITs work with both linux and windows~~ Make most ITs work with both linux and windows Oct 16, 2019

craigdbarber reviewed Oct 16, 2019

View reviewed changes

PR Feedback: Don't swallow exceptions, use Preconditions.checkNotNull…

7f0dc57

…, use powershell to use sleep command. Update documentation for running windows ITs. Remove obsolete windows IT files.

stephenashank force-pushed the feature/issue49windows branch from 6a3d605 to 7f0dc57 Compare October 16, 2019 23:39

Fix exit command for powershell scripts. Make MultipleMatchingConfigu…

5bcb4fc

…rations test more reliable.

stephenashank removed the WIP Work in progress label Oct 17, 2019

craigdbarber approved these changes Oct 17, 2019

View reviewed changes

stephenashank merged commit 0fa6715 into jenkinsci:master Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make most ITs work with both linux and windows #143

Make most ITs work with both linux and windows #143

stephenashank commented Sep 24, 2019

stephenashank Sep 24, 2019

craigdbarber commented Sep 25, 2019

craigdbarber commented Sep 25, 2019

stephenashank commented Sep 26, 2019

craigdbarber Oct 14, 2019

stephenashank Oct 17, 2019

craigdbarber Oct 14, 2019

stephenashank Oct 17, 2019

craigdbarber Oct 14, 2019

stephenashank Oct 14, 2019

craigdbarber Oct 14, 2019

stephenashank Oct 17, 2019

craigdbarber Oct 14, 2019

stephenashank Oct 14, 2019

craigdbarber Oct 14, 2019

stephenashank Oct 16, 2019

craigdbarber Oct 16, 2019

stephenashank Oct 17, 2019

stephenashank Oct 16, 2019

craigdbarber Oct 16, 2019

craigdbarber Oct 16, 2019

stephenashank Oct 17, 2019

craigdbarber Oct 16, 2019

craigdbarber Oct 16, 2019

stephenashank Oct 17, 2019

craigdbarber commented Oct 17, 2019

stephenashank commented Oct 17, 2019

craigdbarber left a comment

		static final String SSH_PRIVATE_KEY = SSH_KEY.getPrivateKey();
		private static final String WINDOWS_STARTUP_SCRIPT =

Make most ITs work with both linux and windows #143

Make most ITs work with both linux and windows #143

Conversation

stephenashank commented Sep 24, 2019

Choose a reason for hiding this comment

craigdbarber commented Sep 25, 2019

craigdbarber commented Sep 25, 2019

stephenashank commented Sep 26, 2019

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

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

craigdbarber commented Oct 17, 2019

stephenashank commented Oct 17, 2019

craigdbarber left a comment

Choose a reason for hiding this comment