Skip to content

Conversation

@karrtikr
Copy link

@karrtikr karrtikr commented Sep 17, 2019

For #7109

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • [ ] Test plan is updated as appropriate
  • [ ] package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • [ ] The wiki is updated with any design decisions/details.
@codecov-io
Copy link

codecov-io commented Sep 17, 2019

Codecov Report

Merging #7442 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #7442 +/- ## ========================================== + Coverage 58.56% 58.57% +<.01%  ========================================== Files 494 494 Lines 21783 21782 -1 Branches 3489 3488 -1 ========================================== Hits 12758 12758 Misses 8236 8236 + Partials 789 788 -1
Impacted Files Coverage Δ
src/client/telemetry/index.ts 86.13% <ø> (ø) ⬆️
src/client/activation/activationService.ts 91.5% <100%> (+0.85%) ⬆️
src/client/telemetry/constants.ts 100% <100%> (ø) ⬆️

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 ecad0ce...5558eba. Read the comment docs.

Copy link

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

None of my comments are critical (1 about a comment and the rest about formatting-for-readability), but please consider addressing them anyway. :)

});
test('Track startup value', async () => {
state.reset();
state
Copy link
Member

Choose a reason for hiding this comment

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

Should be state.setup like in line 605?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I don't understand. Line 605 is

state.setup(s => s.updateValue(TypeMoq.It.isValue(true))).returns(() => Promise.resolve()).verifiable(TypeMoq.Times.once()); 

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant like this:

 state.setup(s => s.value) .returns(() => true) .verifiable(TypeMoq.Times.exactly(2)); state.setup(s => s.updateValue(TypeMoq.It.isAny())) .returns(() => Promise.resolve()) .verifiable(TypeMoq.Times.never());
Kartik Raj and others added 2 commits September 17, 2019 16:41
Co-Authored-By: Eric Snow <ericsnowcurrently@gmail.com>
@karrtikr karrtikr merged commit 9bf27c2 into microsoft:master Sep 18, 2019
@karrtikr karrtikr deleted the lstelemetry branch September 18, 2019 21:33
@lock lock bot locked as resolved and limited conversation to collaborators Sep 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

4 participants