build: Add Store unit tests to Bazel #836
Conversation
| sh_test( | ||
| name = "tautology_test", | ||
| srcs = [":true.sh"], | ||
| filegroup( |
There was a problem hiding this comment.
It isn't clear to me if I should be using the dependencies installed from running Yarn at the root or if I should do something similar to the ngrx_compile_time_deps for tests.
There was a problem hiding this comment.
You have both options.
In the case of tests, your users probably don't expect to build/run these from source. At the same time, your users would download and install the ngrx_compiletime_deps so it's not nice to add test-only dependencies there.
I think I'd probably re-use the node_modules directory installed by yarn (note you can use bazel run @yarn//:yarn to do that with Bazel's version of node and yarn).
There was a problem hiding this comment.
Good deal. This PR follows your suggestion and re-uses the top level node_modules directory for test dependencies.
modules/store/spec/edge.spec.ts
Outdated
| StoreModule.forRoot<TodoAppSchema>({ todos, todoCount } as any) | ||
| ); | ||
| try { | ||
| TestBed.resetTestingModule(); |
There was a problem hiding this comment.
When running these tests via jasmine_node_test the TestBed is not automatically reset.
There was a problem hiding this comment.
I don't know much about TestBed, but I would expect jasmine_node_test just evaluates all scripts, and wouldn't do anything special to reset test state - that would be the job of whatever framework you are testing?
There was a problem hiding this comment.
Can you help me understand what layer was doing this job before?
There was a problem hiding this comment.
Currently Angular's core testing library owns resetting the testing module: https://github.com/angular/angular/blob/master/packages/core/testing/src/before_each.ts#L25
My best guess is that the beforeEach hook Angular is calling belongs to a different Jasmine environment then the one my test is being run under.
There was a problem hiding this comment.
I was able to confirm that the Jasmine environment I'm working with in the bootstrap script is different from the one being used when running my tests. I wasn't able to add a custom reporter in bootstrap, for instance.
|
|
||
| def ts_library(tsconfig = None, node_modules = None, **kwargs): | ||
| if not tsconfig: | ||
| tsconfig = "//:tsconfig.json" | ||
| if not node_modules: | ||
| node_modules = "@ngrx_compiletime_deps//:node_modules" | ||
| _ts_library(tsconfig = tsconfig, node_modules = node_modules, **kwargs) | ||
|
|
||
| def ts_test_library(node_modules = None, **kwargs): |
There was a problem hiding this comment.
Is this an OK pattern of defining my own rules that just set defaults for existing rules similar to what you've done with ts_library? Or should I be more explicit in BUILD files?
There was a problem hiding this comment.
This is a macro (you can tell because you didn't call the rule factory function) and macros can be the gateway to a bad time. In Google, there are a lot of macros that produce surprising combinations of targets, and make the BUILD files look very different from the result of bazel query --output=build.
This usage is okay though, matching the criteria:
- it should only wrap one rule, not multiple
- it should just set defaults
This looks clever and reasonable to me :)
| if not node_modules: | ||
| node_modules = "//:ngrx_test_dependencies" | ||
| if not bootstrap: | ||
| bootstrap = ["ngrx/tools/testing/bootstrap_node_tests.js"] |
There was a problem hiding this comment.
It surprised me that I couldn't pass a label here and instead had to point it to a build artifact.
There was a problem hiding this comment.
It ought to allow "$(location //my:label)" but doesn't yet :(
bazel-contrib/rules_nodejs#32
MikeRyanDev
force-pushed
the
build/add-store-unit-tests-to-bazel
branch
from
0c45769 to
976c356
Compare
|
|
||
| require('zone.js/dist/jasmine-patch.js'); | ||
|
|
||
| TestBed.initTestEnvironment(ServerTestingModule, platformServerTesting()); |
There was a problem hiding this comment.
maybe in this spot you could install jasmine lifecycle hooks so that you don't need to worry about resetting the TestBed and whatnot?
I've used https://jasmine.github.io/2.1/custom_reporter.html before (but it's smelly to have a reporter messing with test state... maybe there's another way?)
There was a problem hiding this comment.
I got around the issue for now by overriding Angular's TestBed.configureTestingModule to always reset the testing module before reconfiguring it.
MikeRyanDev
force-pushed
the
build/add-store-unit-tests-to-bazel
branch
from
976c356 to
fc09773
Compare
MikeRyanDev
changed the title
MikeRyanDev
force-pushed
the
build/add-store-unit-tests-to-bazel
branch
from
fc09773 to
fc199a8
Compare
|
LGTM |
|
Mike, any action items for me as follow-ups to this PR? Does this all work
the way you'd expect, or should we fix something around the
bootstrap/jasmine/beforeEach ergonomics?
…On Sun, Feb 18, 2018 at 1:16 PM Brandon ***@***.***> wrote:
LGTM
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#836 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAC5I9A4-7zYru25ek3MlY1LwcU_3bgMks5tWJMbgaJpZM4SJejd>
.
|
|
@alexeagle I do think the issue with Angular's testing package and Jasmine is a real bug. We have a temporary fix working for us right now. |
|
Okay, could you file that somewhere? I'll probably need to enlist some help
on the team to understand how this is meant to work.
/cc @vikerman
…On Tue, Feb 20, 2018 at 7:55 AM Mike Ryan ***@***.***> wrote:
@alexeagle <https://github.com/alexeagle> I do think the issue with
Angular's testing package and Jasmine is a real bug. We have a temporary
fix working for us right now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#836 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAC5Ixrekxm4M2RrJEAx2hx1dC387PDbks5tWurZgaJpZM4SJejd>
.
|
No description provided.