Use Bazel for repo unit tests #11749

filipesilva · 2018-08-02T12:21:53Z

No description provided.

alexeagle · 2018-08-14T20:09:14Z

.circleci/config.yml

@@ -127,6 +154,9 @@ workflows:
          requires:
            - lint
            - validate
+      - test-bazel:


it's not wise to have separate build and test jobs for bazel. you guarantee that the first test can't start running until the slowest build step is finished, so the total time is longer. Better to let bazel be the only scheduler for its actions

Moved into the same job.

alexeagle · 2018-08-14T20:09:40Z

BUILD

+            # e.g. node_modules/xpath/docs/function resolvers.md
+            "node_modules/**/docs/**",
+            # e.g. node_modules/puppeteer/.local-chromium/mac-536395/chrome-mac/Chromium.app/Contents/Versions/66.0.3347.0/Chromium Framework.framework/Chromium Framework
+            "node_modules/**/.*/**"


we now also add an exclude for spaces
node_modules/**/* *

Added that one too.

alexeagle · 2018-08-14T20:10:18Z

WORKSPACE

@@ -1,52 +1,73 @@
-workspace(name = "angular_devkit")
+workspace(name = "angular_cli")


note, this change has downstream implications for bazel usage (in angular/angular we load this workspace to get build optimizer)

Hadn't considered that. Perhaps it's better to leave it as is. WDYT?

alexeagle · 2018-08-14T20:10:39Z

WORKSPACE


+BAZEL_SKYLIB_VERSION = "0.3.1"


can remove this now with rules_typescript 0.16.1

alexeagle · 2018-08-14T20:11:32Z

packages/angular_devkit/architect/BUILD

@@ -12,28 +12,19 @@ package(default_visibility = ["//visibility:public"])
 ts_library(
    name = "architect",
    srcs = glob(
-        ["src/**/*.ts"],
-        # Currently, this library is used only with the rollup plugin.


why remove the comment? was it stale?

The comment is not applicable at all. It looks like it was copy pasted from the build-optimizer BUILD file.

alexeagle · 2018-08-14T20:12:18Z

packages/angular_devkit/architect/BUILD

    data = glob(["**/*.json"]),
    deps = [
        "//packages/angular_devkit/core",
        "//packages/angular_devkit/core:node",
        "@rxjs",
        "@rxjs//operators",
    ],
-    tsconfig = "//:tsconfig.json",
-    module_name = "@angular-devkit/architect",
-    module_root = "src",


why do we no longer need module root?

We do, but it's added above (before data).

alexeagle · 2018-08-15T13:40:46Z

.circleci/config.yml

@@ -123,7 +142,7 @@ workflows:
          requires:
            - lint
            - validate
-      - build-bazel:
+      - bazel:


Needs update here too
https://github.com/angular/angular-cli/blob/master/.github/angular-robot.yml#L46

alexeagle · 2018-08-15T13:42:45Z

BUILD

+
+load("@com_github_bazelbuild_buildtools//buildifier:def.bzl", "buildifier")
+
+buildifier(


does this work all the way now?
I'm curious if it picks up .bzl files too
If this approach works well, I might want to update my other repos

It worked locally when I hacked buildifier to work on windows at all.

Can I ask you to test though? I haven't tested on linux/osx. Plus the hacks to make it work on windows involve setting absolute paths and it's always a bit of a hassle.

I tried testing it and it made edits.
Can we move buildifier to a separate PR?

alexeagle · 2018-08-15T13:43:52Z

WORKSPACE

 )

-load("@build_bazel_rules_nodejs//:defs.bzl", "check_bazel_version", "node_repositories")
+load("@build_bazel_rules_nodejs//:package.bzl", "rules_nodejs_dependencies")
+rules_nodejs_dependencies()


no need to load nodejs dependencies, the rules_typescript_dependencies() will do it

Isn't it better to avoid relying on that transitively though? Inside this project we don't know what rules_typescript depends on, but we depend directly on rules_nodejs for node_repositories and jasmine_node_test (maybe others too).

the advantage is we're sure to get a version of rules_nodejs that works with rules_typescript.

Unlike under NPM, we don't get two versions of rules_nodejs installed if us and one of our deps disagree on the version. So it's our responsibility to get working versions of both, and I think that's easier if we use the same rules_nodejs version as rules_typescript declares a dependency on.

I'll add a note that we're using it transitively then.

alexeagle · 2018-08-15T13:48:45Z

WORKSPACE

+# rules_typescript_dependencies() also tries to load it but we add it explicitely so we
+# don't have hidden dependencies. 
+# This also means we need to load it before rules_typescript_dependencies().
+RULES_GO_VERSION = "0.14.0"


picking out the versions into variables doesn't seem that useful to me, I've avoided it so we can set up renovate to update automatically (I don't think it understands this)

many repos use git_repository, I avoided that because I thought it might fetch the whole git history and therefore wasteful, but I don't know that for sure. It has the advantage of the version appearing once instead of twice.

I was using git_repository before but asked @gregmagolan and he mentioned that it had been historically flaky (@gregmagolan correct me if I'm wrong).

I think it makes sense to ditch the variable if Renovate doesn't play ball with it though.

alexeagle · 2018-08-15T13:49:25Z

WORKSPACE

-go_rules_dependencies()
-
-go_register_toolchains()
+# # We get Buildifier from here.


delete comments

alexeagle · 2018-08-15T13:50:11Z

packages/angular_devkit/build_optimizer/BUILD

+    deps = [
+        ":build_optimizer",
+        "//packages/angular_devkit/core",
+        # @typings: jasmine


will be interesting to sync this to google3 and see if all the tests pass there too

alexeagle · 2018-08-15T13:53:32Z

tools/defaults.bzl

-  if not tsconfig:
-    tsconfig = DEFAULT_TSCONFIG
-  _ts_library(tsconfig = tsconfig, **kwargs)
+    if not tsconfig:


actually you can remove this now, ts_library#tsconfig defaults to //:tsconfig.json as of release 0.15 or so

hansl · 2018-08-20T17:25:17Z

packages/angular_devkit/schematics/tasks/tslint-fix/executor_spec.ts

@@ -52,7 +52,7 @@ describe('TsLintTaskExecutor', () => {
      testRunner.runSchematicAsync('run-task', null, tree),
      new Observable<void>(obs => {
        process.chdir(getSystemPath(host.root));
-        testRunner.logger.subscribe(x => messages.push(x.message));
+        testRunner.logger.subscribe((x: logging.LogEntry) => messages.push(x.message));


Why do we need this change? logger is already a Observable<LogEntry>, so curious as to why TypeScript doesn't infer this.

When made the initial PR this was not being inferred correctly, and I was not sure why. Tried again now and it wasn't a problem anymore. Maybe an older version of tooling?

…aths

It stops the function from being imported in the unit test.

qiyigg · 2018-08-24T20:49:57Z

packages/angular_devkit/schematics/BUILD

        ],
    ),
    module_name = "@angular-devkit/schematics/tasks",
-    module_root = "tasks",
-    tsconfig = "//:tsconfig.json",
+    module_root = "tasks/index.d.ts",


Changing it to index.d.ts will hide the tasks/node, is that on purpose?

I'm not sure what you mean, can you elaborate? I didn't mean to hide any package.

tasks/index.ts only export all the tasks, the task executors are exported through tasks/node
Changing module_root to "tasks/index.d.ts" will make tasks/node is not reachable.

@qiyigg #12008 should make them available again, sorry about that.

Cool, thanks!

angular-automatic-lock-bot · 2019-09-12T14:40:40Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

filipesilva added the state: WIP label Aug 2, 2018

googlebot added the cla: yes label Aug 2, 2018

filipesilva force-pushed the bazel-tests branch 10 times, most recently from f3f256a to 0795013 Compare August 7, 2018 15:18

filipesilva force-pushed the bazel-tests branch 8 times, most recently from 2f8d914 to 42b5b39 Compare August 14, 2018 16:49

alexeagle reviewed Aug 14, 2018

View reviewed changes

filipesilva force-pushed the bazel-tests branch from 42b5b39 to 1f36fe9 Compare August 15, 2018 11:07

filipesilva added target: patch This PR is targeted for the next patch release and removed state: WIP labels Aug 15, 2018

alexeagle previously approved these changes Aug 15, 2018

View reviewed changes

filipesilva dismissed alexeagle’s stale review via 5705974 August 15, 2018 15:50

filipesilva force-pushed the bazel-tests branch 2 times, most recently from 5705974 to 71b811b Compare August 20, 2018 09:11

hansl reviewed Aug 20, 2018

View reviewed changes

filipesilva force-pushed the bazel-tests branch from 71b811b to 7881d93 Compare August 20, 2018 17:51

filipesilva force-pushed the bazel-tests branch 3 times, most recently from a99726b to f1a7c81 Compare August 21, 2018 09:32

alexeagle previously approved these changes Aug 21, 2018

View reviewed changes

filipesilva added 18 commits August 21, 2018 18:49

build: update bazel workspace

7ad0d0b

ci: update circleci to use bazel 0.15

34fad75

ci: update bazel-build to run tests

166dc51

build: use a more restrictive node modules glob

a2d999d

test(@angular-devkit/core): add bazel tests

bb9c5c3

test(@angular-devkit/schematics): add bazel tests

c1b7a08

refactor(@angular-devkit/schematics-cli): update bazel build file

bf0d240

test(@angular-devkit/build-optimizer): add bazel tests

6687593

test(@schematics/angular): add bazel tests

515d4ca

test(@schematics/update): add bazel tests

534df70

build: include rxjs dependency needed for bazel local repo

f660e3a

build: update rules_nodejs to include windows fixes

7edb94b

build: simplify bazel dependencies

6a598fe

build: add bazel absolute path fixes

1edc071

build: preserve symlinks for rules_nodejs

c7d86fd

build: re-enable missing bazel tests

8f2f518

build: make resolution logic compatible with Bazel

f4fd44f

fix(@angular-devkit/schematics): correctly resolve absolute windows p…

4bf5321

…aths

filipesilva dismissed alexeagle’s stale review via 4bf5321 August 21, 2018 17:52

filipesilva force-pushed the bazel-tests branch from f1a7c81 to 4bf5321 Compare August 21, 2018 17:52

build: remove marker internal from function

d1d99e5

It stops the function from being imported in the unit test.

hansl merged commit f1f987b into angular:master Aug 22, 2018

qiyigg reviewed Aug 24, 2018

View reviewed changes

filipesilva deleted the bazel-tests branch August 24, 2018 22:50

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

		@@ -1,52 +1,73 @@
		workspace(name = "angular_devkit")
		workspace(name = "angular_cli")


		load("@com_github_bazelbuild_buildtools//buildifier:def.bzl", "buildifier")

		buildifier(


		BAZEL_SKYLIB_VERSION = "0.3.1"

Use Bazel for repo unit tests #11749

Use Bazel for repo unit tests #11749

Uh oh!

Conversation

filipesilva commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 12, 2019

Uh oh!

Uh oh!