From 083151366e98815798f2696ad9d1b41c247b933d Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Tue, 11 Jun 2019 18:45:12 -0700 Subject: [PATCH] build(builtin): cleanup //internal/npm_install/test:test Fixes local development issue on Windows where this test would fail on 2nd run due BUILD to files left on disk by the build file generator. Since Windows has no runfiles, the build file generator was running in the sources folder --- .bazelignore | 2 +- .circleci/config.yml | 3 +- .gitignore | 1 - WORKSPACE | 40 ++++++++++++++++++- internal/npm_install/generate_build_file.js | 10 ++++- internal/npm_install/test/BUILD.bazel | 10 +---- internal/npm_install/test/check.js | 19 ++++----- .../test/generate_build_file.spec.js | 5 +-- .../test/golden/BUILD.bazel.golden | 31 ++++++++++++++ .../npm_install/test/golden/WORKSPACE.golden | 1 + .../install_bazel_dependencies.bzl.golden | 2 + .../golden/manual_build_file_contents.golden | 32 +++++++++++++++ .../node_modules/rxjs/BUILD.bazel.golden | 1 - .../test/{package => }/package.json | 3 -- .../test/package/delete_build_files.js | 25 ------------ internal/npm_install/test/update_golden.js | 3 +- .../npm_install/test/{package => }/yarn.lock | 0 17 files changed, 127 insertions(+), 61 deletions(-) create mode 100644 internal/npm_install/test/golden/WORKSPACE.golden create mode 100644 internal/npm_install/test/golden/install_bazel_dependencies.bzl.golden create mode 100755 internal/npm_install/test/golden/manual_build_file_contents.golden rename internal/npm_install/test/{package => }/package.json (80%) delete mode 100644 internal/npm_install/test/package/delete_build_files.js rename internal/npm_install/test/{package => }/yarn.lock (100%) diff --git a/.bazelignore b/.bazelignore index 0bfcff7c65..5051cf280e 100644 --- a/.bazelignore +++ b/.bazelignore @@ -1,6 +1,6 @@ node_modules examples/program/node_modules -internal/npm_install/test/package/node_modules +internal/npm_install/test/node_modules dist # Each e2e test is a nested workspace e2e/ diff --git a/.circleci/config.yml b/.circleci/config.yml index 315344ad3d..55008a3a03 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -11,7 +11,7 @@ # If you change the `default_docker_image` version, also change the `cache_key` version var_1: &default_docker_image circleci/node:10.12 var_2: &browsers_docker_image circleci/node:10.12-browsers -var_3: &cache_key node-0.12-{{ checksum "yarn.lock" }}-{{ checksum "examples/program/yarn.lock" }}-{{ checksum "internal/npm_install/test/package/yarn.lock" }}-2 +var_3: &cache_key node-0.12-{{ checksum "yarn.lock" }}-{{ checksum "examples/program/yarn.lock" }} var_4: &init_environment run: @@ -105,7 +105,6 @@ jobs: paths: - "node_modules" - "examples/program/node_modules" - - "internal/npm_install/test/package/node_modules" # Persist any changes at this point to be reused by further jobs. - persist_to_workspace: diff --git a/.gitignore b/.gitignore index 695c1e66ce..6086e1a894 100644 --- a/.gitignore +++ b/.gitignore @@ -6,7 +6,6 @@ bazel-out /examples/*/bazel-* /packages/*/bazel-* /internal/e2e/*/bazel-* -/internal/npm_install/test/package/* node_modules !/internal/npm_install/test/golden/node_modules examples/vendored_node/node-v10.12.0-linux-x64 diff --git a/WORKSPACE b/WORKSPACE index 05f52b9dbd..8fbe9132de 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -73,9 +73,7 @@ node_repositories( package_json = [ "//:package.json", "@examples_program//:package.json", - "//internal/npm_install/test:package/package.json", ], - preserve_symlinks = True, ) yarn_install( @@ -147,6 +145,44 @@ yarn_install( yarn_lock = "//internal/e2e/fine_grained_no_bin:yarn.lock", ) +yarn_install( + name = "npm_install_test", + manual_build_file_contents = """ +filegroup( + name = "test_files", + srcs = [ + "//:BUILD.bazel", + "//:install_bazel_dependencies.bzl", + "//:manual_build_file_contents", + "//:WORKSPACE", + "//@angular/core:BUILD.bazel", + "//@gregmagolan:BUILD.bazel", + "//@gregmagolan/test-a/bin:BUILD.bazel", + "//@gregmagolan/test-a:BUILD.bazel", + "//@gregmagolan/test-b/bin:BUILD.bazel", + "//@gregmagolan/test-b:BUILD.bazel", + "//ajv:BUILD.bazel", + "//jasmine/bin:BUILD.bazel", + "//jasmine:BUILD.bazel", + "//rxjs:BUILD.bazel", + "//unidiff/bin:BUILD.bazel", + "//unidiff:BUILD.bazel", + "//zone.js:BUILD.bazel", + "//node_modules/@angular/core:BUILD.bazel", + "//node_modules/@gregmagolan:BUILD.bazel", + "//node_modules/@gregmagolan/test-a:BUILD.bazel", + "//node_modules/@gregmagolan/test-b:BUILD.bazel", + "//node_modules/ajv:BUILD.bazel", + "//node_modules/jasmine:BUILD.bazel", + "//node_modules/rxjs:BUILD.bazel", + "//node_modules/unidiff:BUILD.bazel", + "//node_modules/zone.js:BUILD.bazel", + ], +)""", + package_json = "//internal/npm_install/test:package.json", + yarn_lock = "//internal/npm_install/test:yarn.lock", +) + load("@bazel_toolchains//rules:rbe_repo.bzl", "rbe_autoconfig") # Creates toolchain configuration for remote execution with BuildKite CI diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js index 92b3ea6933..122eb2cdba 100644 --- a/internal/npm_install/generate_build_file.js +++ b/internal/npm_install/generate_build_file.js @@ -702,7 +702,15 @@ function filterFiles(files, exts = []) { return false; }) } - return files; + // Filter out internal files + return files.filter(file => { + const basenameUc = path.basename(file).toUpperCase(); + if (basenameUc === '_WORKSPACE' || basenameUc === '_BUILD' || basenameUc === '_BUILD.BAZEL' || + basenameUc === '_BAZEL_MARKER') { + return false; + } + return true; + }); } /** diff --git a/internal/npm_install/test/BUILD.bazel b/internal/npm_install/test/BUILD.bazel index e63a5bf8d6..a2c1e9ecbb 100644 --- a/internal/npm_install/test/BUILD.bazel +++ b/internal/npm_install/test/BUILD.bazel @@ -1,10 +1,5 @@ load("@build_bazel_rules_nodejs//:defs.bzl", "jasmine_node_test", "nodejs_binary") -filegroup( - name = "package", - srcs = glob(["package/**"]), -) - filegroup( name = "goldens", srcs = glob(["golden/**"]), @@ -16,10 +11,10 @@ jasmine_node_test( data = [ ":check.js", ":goldens", - ":package", "//internal/npm_install:generate_build_file", "@npm//jasmine", "@npm//unidiff", + "@npm_install_test//:test_files", ], ) @@ -28,11 +23,10 @@ nodejs_binary( data = [ ":check.js", ":goldens", - ":package", ":update_golden.js", - "//internal/npm_install:generate_build_file", "@npm//jasmine", "@npm//unidiff", + "@npm_install_test//:test_files", ], entry_point = ":update_golden.js", install_source_map_support = False, diff --git a/internal/npm_install/test/check.js b/internal/npm_install/test/check.js index d9d618d1a3..533206d75d 100644 --- a/internal/npm_install/test/check.js +++ b/internal/npm_install/test/check.js @@ -1,21 +1,14 @@ -const generator = require('../generate_build_file'); const fs = require('fs'); const path = require('path'); 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.posix.join(path.dirname(__filename), 'package')); - - // Run the BUILD file generator - generator.main(); -} +const actualFolder = path.posix.join(path.dirname(__filename), '../../../../npm_install_test'); +const goldenFolder = path.posix.join(path.dirname(__filename), 'golden'); function check(file, updateGolden = false) { // Strip comments from generated file for comparison to golden // to make comparison less brittle - const actual = path.posix.join(path.dirname(__filename), 'package', file); + const actual = path.posix.join(actualFolder, file); const actualContents = fs.readFileSync(actual, {encoding: 'utf-8'}) .replace(/\r\n/g, '\n') @@ -28,7 +21,7 @@ function check(file, updateGolden = false) { .replace(/[\n]+/g, '\n'); // Load the golden file for comparison - const golden = path.posix.join(path.dirname(__filename), 'golden', file + '.golden'); + const golden = path.posix.join(goldenFolder, file + '.golden'); if (updateGolden) { // Write to golden file @@ -56,10 +49,12 @@ Update the golden file: } module.exports = { - runGenerator, check, files: [ 'BUILD.bazel', + 'install_bazel_dependencies.bzl', + 'manual_build_file_contents', + 'WORKSPACE', '@angular/core/BUILD.bazel', '@gregmagolan/BUILD.bazel', '@gregmagolan/test-a/bin/BUILD.bazel', diff --git a/internal/npm_install/test/generate_build_file.spec.js b/internal/npm_install/test/generate_build_file.spec.js index 9e4155d5b6..08fdae04ef 100644 --- a/internal/npm_install/test/generate_build_file.spec.js +++ b/internal/npm_install/test/generate_build_file.spec.js @@ -1,9 +1,8 @@ -const {runGenerator, check, files} = require('./check'); +const {check, files} = require('./check'); const {printPackage} = require('../generate_build_file'); describe('build file generator', () => { describe('integration test', () => { - runGenerator(); files.forEach(file => { it(`should produce a BUILD file for ${file}`, () => { check(file); @@ -43,7 +42,7 @@ describe('build file generator', () => { expect(printPackage({...pkg, _files: [], bin: {}})).not.toContain('nodejs_binary'); }); - it('bin entry is an object with an empth path', () => { + it('bin entry is an object with an empty path', () => { expect(printPackage( {...pkg, _files: [], bin: {empty_string: '', _null: null, _undefined: undefined}})) .not.toContain('nodejs_binary'); diff --git a/internal/npm_install/test/golden/BUILD.bazel.golden b/internal/npm_install/test/golden/BUILD.bazel.golden index 826baadc84..734122cb6a 100644 --- a/internal/npm_install/test/golden/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/BUILD.bazel.golden @@ -60,3 +60,34 @@ node_module_library( "//node_modules/@gregmagolan/test-b:test-b__contents", ], ) +filegroup( + name = "test_files", + srcs = [ + "//:BUILD.bazel", + "//:install_bazel_dependencies.bzl", + "//:manual_build_file_contents", + "//:WORKSPACE", + "//@angular/core:BUILD.bazel", + "//@gregmagolan:BUILD.bazel", + "//@gregmagolan/test-a/bin:BUILD.bazel", + "//@gregmagolan/test-a:BUILD.bazel", + "//@gregmagolan/test-b/bin:BUILD.bazel", + "//@gregmagolan/test-b:BUILD.bazel", + "//ajv:BUILD.bazel", + "//jasmine/bin:BUILD.bazel", + "//jasmine:BUILD.bazel", + "//rxjs:BUILD.bazel", + "//unidiff/bin:BUILD.bazel", + "//unidiff:BUILD.bazel", + "//zone.js:BUILD.bazel", + "//node_modules/@angular/core:BUILD.bazel", + "//node_modules/@gregmagolan:BUILD.bazel", + "//node_modules/@gregmagolan/test-a:BUILD.bazel", + "//node_modules/@gregmagolan/test-b:BUILD.bazel", + "//node_modules/ajv:BUILD.bazel", + "//node_modules/jasmine:BUILD.bazel", + "//node_modules/rxjs:BUILD.bazel", + "//node_modules/unidiff:BUILD.bazel", + "//node_modules/zone.js:BUILD.bazel", + ], +) \ No newline at end of file diff --git a/internal/npm_install/test/golden/WORKSPACE.golden b/internal/npm_install/test/golden/WORKSPACE.golden new file mode 100644 index 0000000000..9253255d3f --- /dev/null +++ b/internal/npm_install/test/golden/WORKSPACE.golden @@ -0,0 +1 @@ +workspace(name = "npm_install_test") diff --git a/internal/npm_install/test/golden/install_bazel_dependencies.bzl.golden b/internal/npm_install/test/golden/install_bazel_dependencies.bzl.golden new file mode 100644 index 0000000000..f4bd6eb84d --- /dev/null +++ b/internal/npm_install/test/golden/install_bazel_dependencies.bzl.golden @@ -0,0 +1,2 @@ +def install_bazel_dependencies(): + """Installs all workspaces listed in bazelWorkspaces of all npm packages""" diff --git a/internal/npm_install/test/golden/manual_build_file_contents.golden b/internal/npm_install/test/golden/manual_build_file_contents.golden new file mode 100755 index 0000000000..b18fcdc6a8 --- /dev/null +++ b/internal/npm_install/test/golden/manual_build_file_contents.golden @@ -0,0 +1,32 @@ + +filegroup( + name = "test_files", + srcs = [ + "//:BUILD.bazel", + "//:install_bazel_dependencies.bzl", + "//:manual_build_file_contents", + "//:WORKSPACE", + "//@angular/core:BUILD.bazel", + "//@gregmagolan:BUILD.bazel", + "//@gregmagolan/test-a/bin:BUILD.bazel", + "//@gregmagolan/test-a:BUILD.bazel", + "//@gregmagolan/test-b/bin:BUILD.bazel", + "//@gregmagolan/test-b:BUILD.bazel", + "//ajv:BUILD.bazel", + "//jasmine/bin:BUILD.bazel", + "//jasmine:BUILD.bazel", + "//rxjs:BUILD.bazel", + "//unidiff/bin:BUILD.bazel", + "//unidiff:BUILD.bazel", + "//zone.js:BUILD.bazel", + "//node_modules/@angular/core:BUILD.bazel", + "//node_modules/@gregmagolan:BUILD.bazel", + "//node_modules/@gregmagolan/test-a:BUILD.bazel", + "//node_modules/@gregmagolan/test-b:BUILD.bazel", + "//node_modules/ajv:BUILD.bazel", + "//node_modules/jasmine:BUILD.bazel", + "//node_modules/rxjs:BUILD.bazel", + "//node_modules/unidiff:BUILD.bazel", + "//node_modules/zone.js:BUILD.bazel", + ], +) \ No newline at end of file diff --git a/internal/npm_install/test/golden/node_modules/rxjs/BUILD.bazel.golden b/internal/npm_install/test/golden/node_modules/rxjs/BUILD.bazel.golden index c0b791f379..51525ec69f 100644 --- a/internal/npm_install/test/golden/node_modules/rxjs/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/node_modules/rxjs/BUILD.bazel.golden @@ -2793,7 +2793,6 @@ filegroup( ":src/SubjectSubscription.ts", ":src/Subscriber.ts", ":src/Subscription.ts", - ":src/_WORKSPACE", ":src/add/observable/bindCallback.ts", ":src/add/observable/bindNodeCallback.ts", ":src/add/observable/combineLatest.ts", diff --git a/internal/npm_install/test/package/package.json b/internal/npm_install/test/package.json similarity index 80% rename from internal/npm_install/test/package/package.json rename to internal/npm_install/test/package.json index 1ddc4d9150..a0f57e5584 100644 --- a/internal/npm_install/test/package/package.json +++ b/internal/npm_install/test/package.json @@ -10,8 +10,5 @@ "rxjs": "^6.4.0", "unidiff": "0.0.4", "zone.js": "^0.9.0" - }, - "scripts": { - "postinstall": "node delete_build_files.js" } } diff --git a/internal/npm_install/test/package/delete_build_files.js b/internal/npm_install/test/package/delete_build_files.js deleted file mode 100644 index 8004e71a88..0000000000 --- a/internal/npm_install/test/package/delete_build_files.js +++ /dev/null @@ -1,25 +0,0 @@ -/** - * @fileoverview - * BUILD files in node modules have to be deleted because runfiles are not used - * on Windows. On Windows, the `package` glob sees *all* files because - * `generate_build_file.js` deletes BUILD files from the actual source. - * On other systems, the glob does not cross package boundary, so only a subset - * of files are included in runfiles. Deleting the Bazel files in postinstall - * allows other systems to match the behavior on Windows. - */ - -const fs = require('fs'); - -const files = [ - "node_modules/rxjs/src/webSocket/BUILD.bazel", - "node_modules/rxjs/src/operators/BUILD.bazel", - "node_modules/rxjs/src/testing/BUILD.bazel", - "node_modules/rxjs/src/BUILD.bazel", - "node_modules/rxjs/src/ajax/BUILD.bazel", -]; - -for (const file of files) { - if (fs.existsSync(file)) { - fs.unlinkSync(file); - } -} diff --git a/internal/npm_install/test/update_golden.js b/internal/npm_install/test/update_golden.js index 9165df1644..ce22df4630 100644 --- a/internal/npm_install/test/update_golden.js +++ b/internal/npm_install/test/update_golden.js @@ -1,4 +1,3 @@ -const {runGenerator, check, files} = require('./check'); +const {check, files} = require('./check'); -runGenerator(); files.forEach(file => check(file, true)); diff --git a/internal/npm_install/test/package/yarn.lock b/internal/npm_install/test/yarn.lock similarity index 100% rename from internal/npm_install/test/package/yarn.lock rename to internal/npm_install/test/yarn.lock