Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schematics: Use MockStore in container generated tests #2029

Merged
merged 7 commits into from
Jul 31, 2019

Conversation

jtcrowson
Copy link
Contributor

@jtcrowson jtcrowson commented Jul 22, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

  • Container schematic uses StoreModule for unit tests

Closes #2028

What is the new behavior?

  • Container schematic uses MockStore for unit tests
  • testDepth option to use StoreModule for integration tests instead.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 22, 2019

Preview docs changes for 68facda at https://previews.ngrx.io/pr2029-68facda/

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 22, 2019

Preview docs changes for 689fc93 at https://previews.ngrx.io/pr2029-689fc93/

@brandonroberts
Copy link
Member

What do you think about providing an option to generate an integration test vs a unit test instead of replacing it?

@jtcrowson
Copy link
Contributor Author

jtcrowson commented Jul 23, 2019

I’m up for it. Should we use two separate options?
--spec=true|false, default: true (existing in angular/schematics)
--testDepth=unit|integration, default: unit (new for NgRx/schematics)

@brandonroberts
Copy link
Member

👍

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 24, 2019

Preview docs changes for 896899c at https://previews.ngrx.io/pr2029-896899c/

@jtcrowson
Copy link
Contributor Author

@brandonroberts Added the testDepth option in 896899c and b696b66 👍

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 24, 2019

Preview docs changes for b696b66 at https://previews.ngrx.io/pr2029-b696b66/

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtcrowson seems like a test is failing, could you have a look?
Besides this it LGTM 👍

modules/schematics/src/container/schema.json Outdated Show resolved Hide resolved
modules/schematics/src/container/schema.json Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/schematics/container.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/schematics/container.md Outdated Show resolved Hide resolved
modules/schematics/src/container/index.ts Outdated Show resolved Hide resolved
@jtcrowson
Copy link
Contributor Author

jtcrowson commented Jul 24, 2019

@brandonroberts just curious, why lowercase for the enums? Looks like angular uses uppercase for enums that are words.

🔗https://angular.io/cli/generate
Ex: viewEncapsulation, changeDetection, routingScope

@brandonroberts
Copy link
Member

For the values? The key is testDepth in camel case, which matches the CLI, but the values are lowercase.

@brandonroberts
Copy link
Member

brandonroberts commented Jul 25, 2019

I see it now. Its a mix. I'd rather keep the consistent with lowercase for the values.

@brandonroberts
Copy link
Member

@jtcrowson will you check the failing unit tests?

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 31, 2019

Preview docs changes for b088215 at https://previews.ngrx.io/pr2029-b088215/

@jtcrowson
Copy link
Contributor Author

@brandonroberts sorry for the delay, I've been busy with Angular Denver stuff.

I update the integration/unit casing and made integration the default.

I'm having trouble debugging the test failures since I can't get yarn test:bazel running locally. The CircleCI print out isn't helping much either.

@brandonroberts
Copy link
Member

No problem. I'll take a look at the test failures.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 31, 2019

Preview docs changes for d713cbe at https://previews.ngrx.io/pr2029-d713cbe/

@brandonroberts brandonroberts merged commit 6905d52 into ngrx:master Jul 31, 2019
@jtcrowson
Copy link
Contributor Author

@brandonroberts thanks for d713cbe, makes sense why it was passing with jasmine locally but not with bazel.

@brandonroberts
Copy link
Member

Yep, its weird that it just fails silently also, and doesn't generate the files if it doesn't find the path.

jordanpowell88 pushed a commit to jordanpowell88/platform that referenced this pull request Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schematics: Use MockStore in container generated tests
4 participants