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

Expose @Effect() metadata for testing #493

Merged
merged 11 commits into from
Oct 18, 2017
Merged

Conversation

dinvlad
Copy link
Contributor

@dinvlad dinvlad commented Oct 17, 2017

This PR exposes getEffectsMetadata() method
to access metadata supplied through @Effect() decorators.

This method is useful for when we want to test
that effects have been properly decorated/registered
with the store.

Without this method, it can be hard to ensure that
the decorators were supplied and properly configured
(this could be done through integration tests,
but that would be much more involved,
especially for "negative" cases with dispatch: false).

Relevant commits are 8efba70, f06e744 and 0673d12
(earlier commits have already been merged in #440).

Relevant discussion: #491

Denis Loginov added 8 commits September 29, 2017 02:55
Prior to this fix, collection methods in sorted_state_adapter
ran in quadratic time, because they iterated over the collection
with single-item mutators, each of which also running in linear time.

Now, we first collect all modifications (each in constant time),
then sort them in linearithmic time, and finally merge
them with the existing sorted array linearly.

As a result, we can improve performance of collection methods
to N + M log M, where N is the number of existing items
and M is the number of modifications.

Single-item methods are now expressed through those
for collections, still running linearly.
…_adapter

Removing "holes" created by updateManyMutably()
should be there instead of merge().
It should return "true" unconditionally (probably worth a test).
These methods ran in quadratic time by interating over
the collection and applying a linear algorithm for each item.

Now, we modify entity hashes first (each in constant time),
and then update the array of their ids in linear time.

As a result, all operations now occur linearly.

We also simplified logic for single-item operations
by expressing them through those for collections.

The removal methods also apply to sorted collections
(because they preserve their order).
CircleCI failed the build because includes() is not supported
by its runtime yet. We use an alternative syntax to fix that.
This PR exposes a method to get metadata supplied
through @effect() decorator.

This method is useful for when we want to test
that an effect was properly decorated/registered
in the store.

Related issue: ngrx#491
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 92.498% when pulling ba2eaad on dinvlad:master into 8728bc1 on ngrx:master.

This commit replaces earlier function effectMetadata()
with a map accessor getEffectsMetadata()
and associated EffectsMetadata class.

This syntax enables enables more concise access to metadata
in unit tests.

This is particularly nice, as we can now test
both the observables and their metadata in a similar way:
```
expect(effects.someSource$).toBeObservable(expected);
expect(metadata.someSource$).toEqual({ dispatch: true });
```
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.516% when pulling f06e744 on dinvlad:master into 8728bc1 on ngrx:master.

Earlier version contained incorrect function name.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.516% when pulling 0673d12 on dinvlad:master into 8728bc1 on ngrx:master.

@dinvlad dinvlad changed the title Expose effectMetadata() useful in testing Expose @Effect() metadata for testing Oct 17, 2017
@@ -40,3 +40,19 @@ export const getSourceMetadata = compose(
getEffectMetadataEntries,
getSourceForInstance
);

export interface EffectsMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about changing this to accept a type parameter:

interface EffectsMetadata<T> {
  [keyof T]: undefined | { dispatch: boolean };
}

};
}

export function getEffectsMetadata(instance: any): EffectsMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

And then the signature of this function would become:

function getEffectsMetadata<T>(instance: T): EffectsMetadata<T>

Copy link
Contributor Author

@dinvlad dinvlad Oct 17, 2017

Choose a reason for hiding this comment

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

Makes sense, additional type checking is a plus. Not sure it works with interfaces though, so submitting a change that defines EffectsMetadata as a mapped type.

This change converts EffectsMetadata to a generic type
that requires its keys to be keys of the effects class.

We also update getEffectsMetadata() accordingly.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.516% when pulling 1bc563e on dinvlad:master into 8728bc1 on ngrx:master.

@MikeRyanDev MikeRyanDev merged commit 628b865 into ngrx:master Oct 18, 2017
@MikeRyanDev
Copy link
Member

Thanks!

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