Closed lau32 closed 5 years ago
Merging #430 into master will increase coverage by
0.57%
. The diff coverage isn/a
.
@@ Coverage Diff @@
## master #430 +/- ##
==========================================
+ Coverage 87.77% 88.35% +0.57%
==========================================
Files 67 68 +1
Lines 990 1013 +23
Branches 41 41
==========================================
+ Hits 869 895 +26
+ Misses 103 100 -3
Partials 18 18
Impacted Files | Coverage Δ | |
---|---|---|
src/app/examples/examples.effects.ts | 91.3% <0%> (ø) |
|
src/app/settings/settings.actions.ts | 100% <0%> (+4.87%) |
:arrow_up: |
.../core/google-analytics/google-analytics.effects.ts | 100% <0%> (+27.27%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 1285a4f...edf56cf. Read the comment docs.
@lau32 thanks for your contribution! Let's see what Tim has to say before merging though ;)
Seems good to me!
What are the problems you encountered with the changeLanguage
effect?
Sorry, I meant to write setTranslateServiceLanguage
effect.
It seams that if create call the setTranslateServiceLanguage
with the store, after mocking the pipe on it like so: store.pipe.and.returnValue(of(settings));
, the translateService.use
method isn't being called.
Which is fair, because it will pass the entire pipe logic inside the examples effect.
But, how can I test that setTranslateServiceLanguage
is being called when subscribing to setTranslateServiceLanguage
?
Ah I see, I think mocking the pipe
won't do much as you already are experiencing.
I would suppose the trigger would trigger if we change the store state, more specific the settings. Because selectSettingsState
would execute and emit a new value.
What do you think, can you try this out?
If it isn't working you don't have to spend a lot of time on it, we'll merge this PR and I'll be taking a look for the setTranslateServiceLanguage
test later.
@timdeschryver @lau32 what is the status here? Can I merge or is some changes still coming later?
@tomastrajan you can merge this PR. I would look at another way to trigger the setTranslateServiceLanguage
effect that would not require to create an instance of the store.
Thanks @lau32 !
Aslo, you might consider also running npm run contributors:add -- lau32 test
and npm run contributors:generate
and create a new PR PR to appear in the contributors list ;)
Add tests for settings actions and reducers, examples effects and google-analytics effects.
@timdeschryver, wasn't able to test the
changeLanguage
effect from the examples module. Could you provide some guidance on that please?