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

fix(store): silence console hints in tests #706

Merged
merged 5 commits into from
Dec 16, 2018

Conversation

markwhitfeld
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Currently when tests are run with NGXS the console is filled with noise regarding the hint to turn on the developmentMode during Angular dev mode.
Issue Number: #701

What is the new behavior?

When NGXS is being run during tests the console hints will be supressed.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

@markwhitfeld
Copy link
Member Author

Waiting for feedback from the angular team on my approach to checking if Angular is in Test mode.
https://twitter.com/MarkWhitfeld/status/1071472090920992773

@markwhitfeld
Copy link
Member Author

markwhitfeld commented Dec 9, 2018

Also still trying to figure out how to write tests for this!

@markwhitfeld markwhitfeld force-pushed the fix-issue-701-silent-in-tests branch 4 times, most recently from cd65d12 to 18a65ec Compare December 9, 2018 20:21
@@ -34,6 +35,8 @@ export class InternalStateOperations {
}

private verifyDevMode() {
if (isAngularInTestMode()) return;
Copy link
Member

Choose a reason for hiding this comment

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

@markwhitfeld @eranshmil

I think this is a difficult operation if the number of iterations in this method is more than it seemed. I think this is a difficult operation if the number of iterations in this method is more than it seemed.

{
developmentMode: false,
silentWarning: true // disabled recommend
}

Copy link
Member

Choose a reason for hiding this comment

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

The function is memoized, so it will run only once.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not want to require developers to add options to all their test setups. This would not be a good experience at all. My aim is to reduce the boilerplate required in the tests.

const compilerOptions = platformRef.injector.get(COMPILER_OPTIONS, null);
if (!compilerOptions) return false;
const isInTestMode = compilerOptions.some((item: CompilerOptions) => {
const providers = (item && item.providers) || [];
Copy link
Member

Choose a reason for hiding this comment

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

how hard is the operation? How many iterations will be in production mode?

the number of providers may be a huge?

Copy link
Member Author

Choose a reason for hiding this comment

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

These compiler options will iterate over no more than 10 providers and this list does not get affected by the providers in your app. These are stock standard.

@eranshmil
Copy link
Member

eranshmil commented Dec 10, 2018

@markwhitfeld @splincode
https://github.com/angular/material2/blob/b8c1024a09f89eab39a213fe13f47d529dccdd74/src/lib/core/common-behaviors/common-module.ts#L65
This seems like a cleaner solution.
Tested locally and it works.

@splincode splincode changed the title Fix issue 701 silence console hints in tests fix(store): silence console hints in tests Dec 10, 2018
@splincode
Copy link
Member

@markwhitfeld ping

@markwhitfeld
Copy link
Member Author

@eranshmil Nice find but unfortunately it assumes karma and jasmine:

  private _isTestEnv() {
    const window = this._window as any;
    return window && (window.__karma__ || window.jasmine);
  }

People could be using jest, tape, ava, mocha, etc.

@splincode
Copy link
Member

@markwhitfeld I believe you need to use the solution, which is found @eranshmil

@markwhitfeld
Copy link
Member Author

markwhitfeld commented Dec 11, 2018

So, my tweet received a few interesting answers:
https://twitter.com/MarkWhitfeld/status/1071472090920992773

Some of the options are:

  1. What I have done by looking for the MockNgModuleResolver in the COMPILER _OPTIONS
    My concern is the comments in this file: https://github.com/angular/angular/blob/cf0968f98e844043a0f6c2548201f3c0dfd329a7/packages/compiler/testing/src/testing.ts
  2. platformRef.injector.toString().includes('TestingCompilerFactory');
    Does a toString() on all providers so could be expensive (although we can memoize)
  3. platformRef.injector.toString().includes('coreDynamicTesting');
    Does a toString() on all providers so could be expensive (although we can memoize)
  4. platformRef.injector.get(TestBed, null) !== null;
    Requires a ref to TestBed from @angular/core/testing which is included anyway when you have angular. Apparently this works (even though TestBed is an interface and not a class.
    Could alternatively test for TestingCompilerFactory or coreDynamicTesting in this way too...

Which approach do you prefer?
@eranshmil @splincode @deebloo

@eranshmil
Copy link
Member

For me, the last one seems better than the others.

@splincode
Copy link
Member

good

platformRef.injector.get(TestBed, null) !== null;

@markwhitfeld
Copy link
Member Author

Great, I'll make the change and run it through the tests I wrote for it. Holding thumbs!

@markwhitfeld
Copy link
Member Author

Used the simpler mechanism of checking for the test mode.
Unfortunately the tests I wrote do not apply anymore because the mechanism uses the internal injector and not the platform injector. I have tested it with the integration app and with the local test run.
@splincode please review and merge if happy.

@markwhitfeld markwhitfeld merged commit ddbf001 into ngxs:master Dec 16, 2018
markwhitfeld added a commit that referenced this pull request Dec 16, 2018
* fix: prevent guidance messages in console during tests run

* chore: user simpler mechanism for determining test mode

* fix: injected TestBed must be optional

* chore: silence the recommendation in the integration app

# Conflicts:
#	integration/app/app.module.ts
#	packages/store/src/utils/memoize.ts
@markwhitfeld markwhitfeld added this to the 3.4.0 milestone Dec 19, 2018
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.

3 participants