From 5275d7aa56854142a845a503ba58cabc50f15100 Mon Sep 17 00:00:00 2001
From: Greg Magolan <gmagolan@gmail.com>
Date: Tue, 12 Feb 2019 19:28:27 -0800
Subject: [PATCH] Fix //internal/npm_install/test:test for Windows and
 re-enable Windows tests on bazel CI (#542)

* Fix //internal/npm_install/test:test so that it works on Windows & re-enable Windows tests on BazelCI

* Work around npm_install golden difference between platforms

* Typo

* Fix Windows CI
---
 .bazelci/presubmit.yml                        |  8 +--
 .bazelignore                                  |  1 +
 .gitignore                                    |  1 +
 WORKSPACE                                     |  2 +-
 internal/node/node_repositories.bzl           |  3 +-
 internal/npm_install/generate_build_file.js   | 62 ++++++++++---------
 internal/npm_install/test/BUILD.bazel         | 18 +++---
 internal/npm_install/test/check.js            | 25 +++++---
 .../test/golden/BUILD.bazel.golden            |  1 -
 .../@gregmagolan/test-a/BUILD.bazel.golden    |  1 -
 .../node_modules/jasmine/BUILD.bazel.golden   |  1 -
 .../test/{ => package}/package.json           |  1 -
 .../npm_install/test/{ => package}/yarn.lock  |  5 --
 internal/web_package/BUILD.bazel              |  3 +
 internal/web_package/test/BUILD.bazel         |  3 +
 package.json                                  |  4 +-
 yarn.lock                                     | 27 +++++++-
 17 files changed, 101 insertions(+), 65 deletions(-)
 rename internal/npm_install/test/{ => package}/package.json (85%)
 rename internal/npm_install/test/{ => package}/yarn.lock (95%)

diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml
index 0908034af9..bfc51b353c 100644
--- a/.bazelci/presubmit.yml
+++ b/.bazelci/presubmit.yml
@@ -29,10 +29,10 @@ platforms:
     - "@nodejs//:yarn"
     build_targets:
     - "//..."
-    # FIXME: enable tests on Windows after fixing them
-    # They were accidentally disabled for a while.
-    # test_targets:
-    # - "//..."
+    test_flags:
+    - "--test_tag_filters=-fix-windows"
+    test_targets:
+    - "//..."
   rbe_ubuntu1604:
     build_targets:
     - "//..."
diff --git a/.bazelignore b/.bazelignore
index aec53e94e1..9b04b3e7d3 100644
--- a/.bazelignore
+++ b/.bazelignore
@@ -1,6 +1,7 @@
 node_modules
 # Examples are tested by running examples/test_example.sh outside of Bazel
 examples/
+internal/npm_install/test/package
 internal/e2e/bazel_workspaces
 internal/e2e/bazel_workspaces_compat
 internal/e2e/fine_grained_symlinks
diff --git a/.gitignore b/.gitignore
index ce555c9717..7c8257f716 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,7 @@ rules_nodejs-*.tar.gz
 /bazel-*
 /examples/*/bazel-*
 /internal/e2e/*/bazel-*
+/internal/npm_install/test/package/*
 node_modules
 examples/vendored_node/node-v10.12.0-linux-x64
 examples/vendored_node/node-v10.12.0-linux-x64.tar.xz
diff --git a/WORKSPACE b/WORKSPACE
index a4302c59dd..d65eec99e0 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -52,7 +52,7 @@ node_repositories(
         "//:package.json",
         "@program_example//:package.json",
         "//internal/test:package.json",
-        "//internal/npm_install/test:package.json",
+        "//internal/npm_install/test:package/package.json",
     ],
     preserve_symlinks = True,
 )
diff --git a/internal/node/node_repositories.bzl b/internal/node/node_repositories.bzl
index 4c079f3a91..0c4487f24a 100644
--- a/internal/node/node_repositories.bzl
+++ b/internal/node/node_repositories.bzl
@@ -26,7 +26,7 @@ load(":node_labels.bzl", "get_yarn_node_repositories_label")
 
 # Callers that don't specify a particular version will get these.
 DEFAULT_NODE_VERSION = "10.13.0"
-DEFAULT_YARN_VERSION = "1.12.1"
+DEFAULT_YARN_VERSION = "1.12.3"
 
 # Dictionary mapping NodeJS versions to sets of hosts and their correspoding (filename, strip_prefix, sha256) tuples.
 NODE_REPOSITORIES = {
@@ -67,6 +67,7 @@ NODE_REPOSITORIES = {
 # Dictionary mapping Yarn versions to their correspoding (filename, strip_prefix, sha256) tuples.
 YARN_REPOSITORIES = {
     "1.12.1": ("yarn-v1.12.1.tar.gz", "yarn-v1.12.1", "09bea8f4ec41e9079fa03093d3b2db7ac5c5331852236d63815f8df42b3ba88d"),
+    "1.12.3": ("yarn-v1.12.3.tar.gz", "yarn-v1.12.3", "02cd4b589ec22c4bdbd2bc5ebbfd99c5e99b07242ad68a539cb37896b93a24f2"),
     "1.3.2": ("yarn-v1.3.2.tar.gz", "yarn-v1.3.2", "6cfe82e530ef0837212f13e45c1565ba53f5199eec2527b85ecbcd88bf26821d"),
     "1.5.1": ("yarn-v1.5.1.tar.gz", "yarn-v1.5.1", "cd31657232cf48d57fdbff55f38bfa058d2fb4950450bd34af72dac796af4de1"),
     "1.6.0": ("yarn-v1.6.0.tar.gz", "yarn-v1.6.0", "a57b2fdb2bfeeb083d45a883bc29af94d5e83a21c25f3fc001c295938e988509"),
diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js
index a6ed779674..8b7af9bbcd 100644
--- a/internal/npm_install/generate_build_file.js
+++ b/internal/npm_install/generate_build_file.js
@@ -388,35 +388,39 @@ function listFiles(rootDir, subDir = '') {
   if (!fs.existsSync(dir) || !fs.statSync(dir).isDirectory()) {
     return [];
   }
-  return fs
-      .readdirSync(dir)
-      .reduce((files, file) => {
-        const fullPath = path.posix.join(dir, file);
-        const relPath = path.posix.join(subDir, file);
-        const isSymbolicLink = fs.lstatSync(fullPath).isSymbolicLink();
-        let stat;
-        try {
-          stat = fs.statSync(fullPath);
-        } catch (e) {
-          if (isSymbolicLink) {
-            // Filter out broken symbolic links. These cause fs.statSync(fullPath)
-            // to fail with `ENOENT: no such file or directory ...`
-            return files;
-          }
-          throw e;
-        }
-        const isDirectory = stat.isDirectory();
-        if (isDirectory && isSymbolicLink) {
-          // Filter out symbolic links to directories. An issue in yarn versions
-          // older than 1.12.1 creates symbolic links to folders in the .bin folder
-          // which leads to Bazel targets that cross package boundaries.
-          // See https://github.com/bazelbuild/rules_nodejs/issues/428 and
-          // https://github.com/bazelbuild/rules_nodejs/issues/438.
-          // This is tested in internal/e2e/fine_grained_symlinks.
-          return files;
-        }
-        return isDirectory ? files.concat(listFiles(rootDir, relPath)) : files.concat(relPath);
-      }, []);
+  return fs.readdirSync(dir)
+      .reduce(
+          (files, file) => {
+            const fullPath = path.posix.join(dir, file);
+            const relPath = path.posix.join(subDir, file);
+            const isSymbolicLink = fs.lstatSync(fullPath).isSymbolicLink();
+            let stat;
+            try {
+              stat = fs.statSync(fullPath);
+            } catch (e) {
+              if (isSymbolicLink) {
+                // Filter out broken symbolic links. These cause fs.statSync(fullPath)
+                // to fail with `ENOENT: no such file or directory ...`
+                return files;
+              }
+              throw e;
+            }
+            const isDirectory = stat.isDirectory();
+            if (isDirectory && isSymbolicLink) {
+              // Filter out symbolic links to directories. An issue in yarn versions
+              // older than 1.12.1 creates symbolic links to folders in the .bin folder
+              // which leads to Bazel targets that cross package boundaries.
+              // See https://github.com/bazelbuild/rules_nodejs/issues/428 and
+              // https://github.com/bazelbuild/rules_nodejs/issues/438.
+              // This is tested in internal/e2e/fine_grained_symlinks.
+              return files;
+            }
+            return isDirectory ? files.concat(listFiles(rootDir, relPath)) : files.concat(relPath);
+          },
+          [])
+      // We return a sorted array so that the order of files
+      // is the same regardless of platform
+      .sort();
 }
 
 /**
diff --git a/internal/npm_install/test/BUILD.bazel b/internal/npm_install/test/BUILD.bazel
index 7b5babcd09..ec4524bd56 100644
--- a/internal/npm_install/test/BUILD.bazel
+++ b/internal/npm_install/test/BUILD.bazel
@@ -1,8 +1,8 @@
 load("@build_bazel_rules_nodejs//:defs.bzl", "jasmine_node_test", "nodejs_binary")
 
 filegroup(
-    name = "node_modules",
-    srcs = glob(["node_modules/**"]),
+    name = "package",
+    srcs = glob(["package/**"]),
 )
 
 filegroup(
@@ -16,11 +16,11 @@ jasmine_node_test(
     data = [
         ":check.js",
         ":goldens",
-        ":node_modules",
-        ":yarn.lock",
+        ":package",
+        "//internal/npm_install:generate_build_file",
+        "@npm//jasmine",
+        "@npm//unidiff",
     ],
-    node_modules = ":node_modules",
-    deps = ["//internal/npm_install:generate_build_file"],
 )
 
 nodejs_binary(
@@ -28,12 +28,12 @@ nodejs_binary(
     data = [
         ":check.js",
         ":goldens",
-        ":node_modules",
+        ":package",
         ":update_golden.js",
-        ":yarn.lock",
         "//internal/npm_install:generate_build_file",
+        "@npm//jasmine",
+        "@npm//unidiff",
     ],
     entry_point = "build_bazel_rules_nodejs/internal/npm_install/test/update_golden.js",
     install_source_map_support = False,
-    node_modules = ":node_modules",
 )
diff --git a/internal/npm_install/test/check.js b/internal/npm_install/test/check.js
index 76a6ae91f9..6b52f5cd6b 100644
--- a/internal/npm_install/test/check.js
+++ b/internal/npm_install/test/check.js
@@ -6,24 +6,29 @@ const unidiff = require('unidiff')
 function runGenerator() {
   // We must change the directory to the BUILD file path
   // so the generator is able to run
-  process.chdir(path.dirname(__filename));
+  process.chdir(path.posix.join(path.dirname(__filename), 'package'));
 
   // Run the BUILD file generator
   generator.main();
 }
 
-function check(actual, updateGolden = false) {
+function check(file, updateGolden = false) {
   // Strip comments from generated file for comparison to golden
   // to make comparison less brittle
-  const actualContents = fs.readFileSync(actual, {encoding: 'utf-8'})
-                             .replace(/\r\n/g, '\n')
-                             .split('\n')
-                             .filter(l => !l.trimLeft().startsWith('#'))
-                             .join('\n')
-                             .replace(/[\n]+/g, '\n');
+  const actual = path.posix.join(path.dirname(__filename), 'package', file);
+  const actualContents =
+      fs.readFileSync(actual, {encoding: 'utf-8'})
+          .replace(/\r\n/g, '\n')
+          .split('\n')
+          // Remove all comments for the comparison
+          .filter(l => !l.trimLeft().startsWith('#'))
+          // Remove .cmd files for the comparison since they only exist on Windows
+          .filter(l => !l.endsWith('.cmd",'))
+          .join('\n')
+          .replace(/[\n]+/g, '\n');
 
   // Load the golden file for comparison
-  const golden = path.posix.join('golden', actual + '.golden');
+  const golden = path.posix.join(path.dirname(__filename), 'golden', file + '.golden');
   const goldenContents = fs.readFileSync(golden, {encoding: 'utf-8'}).replace(/\r\n/g, '\n');
 
   // Check if actualContents matches golden file
@@ -36,7 +41,7 @@ function check(actual, updateGolden = false) {
       // Generated does not match golden
       const diff = unidiff.diffLines(goldenContents, actualContents);
       const prettyDiff = unidiff.formatLines(diff);
-      throw new Error(`Actual output in ${actual} doesn't match golden file ${golden}.
+      throw new Error(`Actual output in ${file} doesn't match golden file ${golden}.
 
 Diff:
 ${prettyDiff}
diff --git a/internal/npm_install/test/golden/BUILD.bazel.golden b/internal/npm_install/test/golden/BUILD.bazel.golden
index e106f24f0f..d1f7585606 100644
--- a/internal/npm_install/test/golden/BUILD.bazel.golden
+++ b/internal/npm_install/test/golden/BUILD.bazel.golden
@@ -27,6 +27,5 @@ filegroup(
         "//node_modules/wrappy:wrappy__files",
         "//node_modules/@gregmagolan/test-a:test-a__files",
         "//node_modules/@gregmagolan/test-b:test-b__files",
-        "//node_modules/@yarnpkg/lockfile:lockfile__files",
     ],
 )
diff --git a/internal/npm_install/test/golden/node_modules/@gregmagolan/test-a/BUILD.bazel.golden b/internal/npm_install/test/golden/node_modules/@gregmagolan/test-a/BUILD.bazel.golden
index 0efe4bc2be..881edc16f0 100644
--- a/internal/npm_install/test/golden/node_modules/@gregmagolan/test-a/BUILD.bazel.golden
+++ b/internal/npm_install/test/golden/node_modules/@gregmagolan/test-a/BUILD.bazel.golden
@@ -12,7 +12,6 @@ filegroup(
 filegroup(
     name = "test-a__files",
     srcs = [
-        ":.bin/test",
         ":@bin/test.js",
         ":main.js",
         ":package.json",
diff --git a/internal/npm_install/test/golden/node_modules/jasmine/BUILD.bazel.golden b/internal/npm_install/test/golden/node_modules/jasmine/BUILD.bazel.golden
index cd58c5778d..094243a005 100644
--- a/internal/npm_install/test/golden/node_modules/jasmine/BUILD.bazel.golden
+++ b/internal/npm_install/test/golden/node_modules/jasmine/BUILD.bazel.golden
@@ -23,7 +23,6 @@ filegroup(
 filegroup(
     name = "jasmine__files",
     srcs = [
-        ":.bin/jasmine",
         ":.editorconfig",
         ":.travis.yml",
         ":Gruntfile.js",
diff --git a/internal/npm_install/test/package.json b/internal/npm_install/test/package/package.json
similarity index 85%
rename from internal/npm_install/test/package.json
rename to internal/npm_install/test/package/package.json
index e01e16b6fb..b468eefd0d 100644
--- a/internal/npm_install/test/package.json
+++ b/internal/npm_install/test/package/package.json
@@ -4,7 +4,6 @@
   "devDependencies": {
     "@gregmagolan/test-a": "0.0.4",
     "@gregmagolan/test-b": "0.0.2",
-    "@yarnpkg/lockfile": "1.1.0",
     "ajv": "5.5.2",
     "jasmine": "3.2.0",
     "unidiff": "0.0.4"
diff --git a/internal/npm_install/test/yarn.lock b/internal/npm_install/test/package/yarn.lock
similarity index 95%
rename from internal/npm_install/test/yarn.lock
rename to internal/npm_install/test/package/yarn.lock
index ee9e02275c..0a007df5ff 100644
--- a/internal/npm_install/test/yarn.lock
+++ b/internal/npm_install/test/package/yarn.lock
@@ -19,11 +19,6 @@
   dependencies:
     "@gregmagolan/test-a" "0.0.1"
 
-"@yarnpkg/lockfile@1.1.0":
-  version "1.1.0"
-  resolved "https://registry.yarnpkg.com/@yarnpkg/lockfile/-/lockfile-1.1.0.tgz#e77a97fbd345b76d83245edcd17d393b1b41fb31"
-  integrity sha512-GpSwvyXOcOOlV70vbnzjj4fW5xW/FdUF6nQEt1ENy7m4ZCczi1+/buVUPAqmGfqznsORNFzUMjctTIp8a9tuCQ==
-
 ajv@5.5.2:
   version "5.5.2"
   resolved "https://registry.yarnpkg.com/ajv/-/ajv-5.5.2.tgz#73b5eeca3fab653e3d3f9422b341ad42205dc965"
diff --git a/internal/web_package/BUILD.bazel b/internal/web_package/BUILD.bazel
index 545fffc2e8..7b4396501a 100644
--- a/internal/web_package/BUILD.bazel
+++ b/internal/web_package/BUILD.bazel
@@ -42,6 +42,9 @@ jasmine_node_test(
     name = "assembler_test",
     srcs = ["assembler_spec.js"],
     node_modules = "@build_bazel_rules_nodejs_web_package_deps//:node_modules",
+    tags = [
+        "fix-windows",
+    ],
     deps = [
         "assembler.js",
     ],
diff --git a/internal/web_package/test/BUILD.bazel b/internal/web_package/test/BUILD.bazel
index 00e491d0e9..595359c0c6 100644
--- a/internal/web_package/test/BUILD.bazel
+++ b/internal/web_package/test/BUILD.bazel
@@ -25,4 +25,7 @@ jasmine_node_test(
         ":pkg",
     ],
     node_modules = "@build_bazel_rules_nodejs_web_package_deps//:node_modules",
+    tags = [
+        "fix-windows",
+    ],
 )
diff --git a/package.json b/package.json
index 95dd95c794..9ad6e6d69c 100644
--- a/package.json
+++ b/package.json
@@ -15,8 +15,10 @@
         "@bazel/typescript": "0.23.2",
         "clang-format": "1.2.2",
         "husky": "^0.14.3",
+        "jasmine": "3.3.1",
         "shelljs": "^0.8.2",
-        "typescript": "^3.3.1"
+        "typescript": "^3.3.1",
+        "unidiff": "0.0.4"
     },
     "scripts": {
         "//nested workspace tests": "Run bazel from within the folder containing the WORKSPACE so that the WORKSPACE file is tested",
diff --git a/yarn.lock b/yarn.lock
index b85662bca8..43b366b318 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -146,6 +146,11 @@ decamelize@^1.1.1:
   resolved "https://registry.yarnpkg.com/decamelize/-/decamelize-1.2.0.tgz#f6534d15148269b20352e7bee26f501f9a191290"
   integrity sha1-9lNNFRSCabIDUue+4m9QH5oZEpA=
 
+diff@^2.2.2:
+  version "2.2.3"
+  resolved "https://registry.yarnpkg.com/diff/-/diff-2.2.3.tgz#60eafd0d28ee906e4e8ff0a52c1229521033bf99"
+  integrity sha1-YOr9DSjukG5Oj/ClLBIpUhAzv5k=
+
 fs.realpath@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/fs.realpath/-/fs.realpath-1.0.0.tgz#1504ad2523158caa40db4a2787cb01411994ea4f"
@@ -163,7 +168,7 @@ glob@^7.0.0:
     once "^1.3.0"
     path-is-absolute "^1.0.0"
 
-glob@^7.0.5:
+glob@^7.0.5, glob@^7.0.6:
   version "7.1.3"
   resolved "https://registry.yarnpkg.com/glob/-/glob-7.1.3.tgz#3960832d3f1574108342dafd3a67b332c0969df1"
   integrity sha512-vcfuiIxogLV4DlGBHIUOwI0IbrJ8HWPc4MU7HzviGeNho/UJDfi6B5p3sHeWIQ0KGIU0Jpxi5ZHxemQfLkkAwQ==
@@ -226,6 +231,19 @@ jasmine-core@2.8.0:
   resolved "https://registry.yarnpkg.com/jasmine-core/-/jasmine-core-2.8.0.tgz#bcc979ae1f9fd05701e45e52e65d3a5d63f1a24e"
   integrity sha1-vMl5rh+f0FcB5F5S5l06XWPxok4=
 
+jasmine-core@~3.3.0:
+  version "3.3.0"
+  resolved "https://registry.yarnpkg.com/jasmine-core/-/jasmine-core-3.3.0.tgz#dea1cdc634bc93c7e0d4ad27185df30fa971b10e"
+  integrity sha512-3/xSmG/d35hf80BEN66Y6g9Ca5l/Isdeg/j6zvbTYlTzeKinzmaTM4p9am5kYqOmE05D7s1t8FGjzdSnbUbceA==
+
+jasmine@3.3.1:
+  version "3.3.1"
+  resolved "https://registry.yarnpkg.com/jasmine/-/jasmine-3.3.1.tgz#d61bb1dd8888859bd11ea83074a78ee13d949905"
+  integrity sha512-/vU3/H7U56XsxIXHwgEuWpCgQ0bRi2iiZeUpx7Nqo8n1TpoDHfZhkPIc7CO8I4pnMzYsi3XaSZEiy8cnTfujng==
+  dependencies:
+    glob "^7.0.6"
+    jasmine-core "~3.3.0"
+
 lcid@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/lcid/-/lcid-1.0.0.tgz#308accafa0bc483a3867b4b6f2b9506251d1b835"
@@ -373,6 +391,13 @@ typescript@^3.3.1:
   resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.3.1.tgz#6de14e1db4b8a006ac535e482c8ba018c55f750b"
   integrity sha512-cTmIDFW7O0IHbn1DPYjkiebHxwtCMU+eTy30ZtJNBPF9j2O1ITu5XH2YnBeVRKWHqF+3JQwWJv0Q0aUgX8W7IA==
 
+unidiff@0.0.4:
+  version "0.0.4"
+  resolved "https://registry.yarnpkg.com/unidiff/-/unidiff-0.0.4.tgz#257fc346ac6f134b0b75d08895320872a737e222"
+  integrity sha1-JX/DRqxvE0sLddCIlTIIcqc34iI=
+  dependencies:
+    diff "^2.2.2"
+
 window-size@^0.1.4:
   version "0.1.4"
   resolved "https://registry.yarnpkg.com/window-size/-/window-size-0.1.4.tgz#f8e1aa1ee5a53ec5bf151ffa09742a6ad7697876"