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

feat(alert): add config service to provide default values #661

Closed
wants to merge 1 commit into from

Conversation

jnizet
Copy link
Member

@jnizet jnizet commented Sep 2, 2016

refs #518

I suffered with the tests of NgbDismissibleAlert because creating an instance of this component using new or with the TestBed is harder than with other components: it has many dependencies thatare used in the constructor. Instead of trying to mock them all or to change the code of the component just to ease that test, I thus used a slightly different test strategy than for the other components.

@pkozlowski-opensource pkozlowski-opensource modified the milestone: alpha.4 Sep 2, 2016
it('should initialize inputs with provided config', () => {
let config: NgbDismissibleAlertConfig;
const fixture = createTestComponent(`<template ngbAlert>Hello, {{name}}!</template>`);
inject([NgbDismissibleAlertConfig], (c: NgbDismissibleAlertConfig) => {
Copy link
Member

Choose a reason for hiding this comment

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

This seems weird to me - does it need to be injected here? Can you explain a little more in detail about what situation this is simulating?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to simulate what the config service documentation suggests to do to globally change the default values for alerts (and all the other components):

  • NgbAlertModule declares an NgbAlertConfig provider
  • an application main module (simulated by the test module here) imports NgbAlertModule (or NgbModule). A global NgbAlertConfig service, containing mutable default values, is thus available to the whole application
  • the root application component gets a reference to this global NgbAlertConfig config thanks to dependency injection. That is simulated by the inject() in the test
  • the root application component sets custom default values in the injected NgbAlertConfig service. That is simulated by the body of the function passed to inject().
  • A template of the application ends up using an NgbAlert component. This is simulated by the createTestComponent() call in the test
  • The created alert should use the default values set before in the NgbAlertConfig service. This is what the test checks.

Does that clarify? I agree the test is more convoluted than the other similar tests, but checking for the right timeout can't be done by just inspecting the dom, and, to be fair, I haven't found a way to just getting an instance of the NgbDismissibleAlert in the test.

Using

TestBed.createComponent(NgbDismissibleAlert)

leads to the following error message, which looks completely wrong to me:

Cannot create the component NgbDismissibleAlert as it was not imported into the testing module!

@wesleycho
Copy link
Member

This LGTM, just want a little more clarification on the test scenario.

@icfantv
Copy link
Member

icfantv commented Sep 2, 2016

@wesleycho are you satisfied with the explanation?
@jnizet please rebase and fix the conflicts. thanks.

@wesleycho
Copy link
Member

Thanks for the very detailed explanation - I'm ok with this then.

@jnizet
Copy link
Member Author

jnizet commented Sep 2, 2016

Rebase done. I also added the missing api doc for the two config service un the demo and fixed the label of the config demo. :-/

@icfantv icfantv closed this in e666c61 Sep 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants