[JENKINS-14807] Fix path separator when EnvVars overrides variable like

PATH+XYZ The getEnvironment(Node, TaskListener) now returns an environment setup correctly with the platform value of the computer node where the job is executed.
jenkinsci · Jul 10, 2017 · cf0183d · cf0183d
1 parent 2145dbf
commit cf0183d
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 6 deletions.
diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java
@@ -374,13 +374,15 @@ public EnvVars getCharacteristicEnvVars() {
      *      (in which case none of the node specific properties would be reflected in the resulting override.)
      */
     public @Nonnull EnvVars getEnvironment(@CheckForNull Node node, @Nonnull TaskListener listener) throws IOException, InterruptedException {
-        EnvVars env;
+        EnvVars env = new EnvVars();
 
-        if (node!=null) {
+        if (node != null) {
             final Computer computer = node.toComputer();
-            env = (computer != null) ? computer.buildEnvironment(listener) : new EnvVars();                
-        } else {
-            env = new EnvVars();
+            if (computer != null) {
+                // we need to get computer environment to inherit platform details 
+                env = computer.getEnvironment();
+                env.putAll(computer.buildEnvironment(listener));
+            }
         }
 
         env.putAll(getCharacteristicEnvVars());

diff --git a/core/src/test/java/hudson/model/JobTest.java b/core/src/test/java/hudson/model/JobTest.java
@@ -1,9 +1,25 @@
 package hudson.model;
 
-import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.*;
 
+import java.lang.reflect.Method;
+
+import org.hamcrest.CoreMatchers;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.jvnet.hudson.test.Issue;
+import org.mockito.Mockito;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.MockRepository;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import hudson.EnvVars;
+import hudson.Platform;
+import hudson.util.ReflectionUtils;
 
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({ Node.class, Platform.class })
 public class JobTest {
 
     @Test
@@ -27,4 +43,44 @@ public void testSetDisplayNameZeroLength() throws Exception {
         // make sure the getDisplayName returns the project name
         assertEquals(StubJob.DEFAULT_STUB_JOB_NAME, j.getDisplayName());
     }
+
+    @Issue("JENKINS-14807")
+    @Test
+    public void use_slave_platform_path_separator_when_contribute_path() throws Throwable {
+        // mock environment to simulate EnvVars of slave node with different platform than master
+        Platform slavePlatform = Platform.current() == Platform.UNIX ? Platform.WINDOWS : Platform.UNIX;
+        PowerMockito.mockStatic(Platform.class);
+        Mockito.when(Platform.current()).thenReturn(slavePlatform);
+
+        // environments are prepared after mock the Platform.current() method
+        EnvVars emptyEnv = new EnvVars();
+        EnvVars slaveEnv = new EnvVars(EnvVars.masterEnvVars);
+
+        // reset mock of Platform class
+        MockRepository.removeClassMethodInvocationControl(Platform.class);
+
+        Job<?, ?> job = Mockito.mock(FreeStyleProject.class);
+        Mockito.when(job.getEnvironment(Mockito.any(Node.class), Mockito.any(TaskListener.class))).thenCallRealMethod();
+        Mockito.when(job.getCharacteristicEnvVars()).thenReturn(emptyEnv);
+
+        Computer c = Mockito.mock(Computer.class);
+        // ensure that PATH variable exists to perform the path separator join
+        if (!slaveEnv.containsKey("PATH")) {
+            slaveEnv.put("PATH", "/bin/bash");
+        }
+        Mockito.when(c.getEnvironment()).thenReturn(slaveEnv);
+        Mockito.when(c.buildEnvironment(Mockito.any(TaskListener.class))).thenReturn(emptyEnv);
+
+        Node node = PowerMockito.mock(Node.class);
+        PowerMockito.doReturn(c).when(node).toComputer();
+
+        EnvVars env = job.getEnvironment(node, null);
+        String path = "/test";
+        env.override("PATH+TEST", path);
+
+        assertThat("The contributed PATH was not joined using the path separator defined in slave node", //
+                env.get("PATH"), //
+                CoreMatchers.containsString(path + (slavePlatform == Platform.WINDOWS ? ';' : ':')));
+    }
+
 }