Add unit/integration testing for @opencensus/web-instrumentation-zone #104

crdgonzalezca · 2019-06-27T15:08:21Z

Add unit testing for OnPageInteraction class.
Integration testing on InteractionTracker which creates fake interaction in other to test all kind of supported tasks (e.g. setTimeout, Promise, HTTP requests).

Additionally, there are some changes little changes:

Change InteractionTracker as Singleton as there will be only one InteractionTracker instance always.
(Fix) Reset currentEventTracingZone when the interaction has finished before it is reset automatically.
Register a initial SpanEventListener in the Tracing (for this case the NOOP_EXPORTER) as this is helpful for testing.
Stop tracking periodic tasks (setInterval) as they might never stop and interaction wouldn't be stable.

draffensperger · 2019-06-27T15:28:25Z

packages/opencensus-web-instrumentation-zone/src/interaction-tracker.ts


 export class InteractionTracker {
  // Allows to track several events triggered by the same user interaction in the right Zone.
-  private currentEventTracingZone?: Zone;
+  currentEventTracingZone?: Zone;


Why does this need to be exposed to tests?

It's necessary to test when there are overlapping interactions and check the interactions have different ID. Also, when there are several event clicks corresponding to the same interaction, we want to make sure there is not a new zone for those click events. I am open to receive recommendations on a way to avoid exposing this.

Update: I stopped exposing this as final decision was to leave this kind of testing of a future refactoring on InteractionTracker

draffensperger · 2019-06-27T16:58:42Z

packages/opencensus-web-instrumentation-zone/src/interaction-tracker.ts

  }

  /** Increments the count of outstanding tasks for a given interaction id. */
-  private incrementTaskCount(interactionId: string) {
+  incrementTaskCount(interactionId: string) {


What's the motivation for exposing these methods and verifying that they are called?

My intuition would be to try to measure this implicitly in the test via seeing how long the interaction took

The motivation is to make sure we actually count the correct amount of tasks involved in the interaction. The duration of interaction is a good test but might give a wrong testing on the amount of tracked tasks for every interaction, as we actually want to check that, for example, macrotasks are handled. I am open to different approach though.

Well, I guess to me the fact that it is counting tasks is more of an implementation detail that is not exposed to the end user of the tracker. So for an integration level test, I think we should be testing the surface as experienced by the user as close as possible.

Another idea would be to try to split out the part of the code that does the task counting and test that separately more as an implementation detail to make sure the counts are correct. And then in the integration test to just measure the total time and span name/metadata.

Final decision: I decided to stop testing the task count as it should be more an implementation testing more than integration testing. Also, this could be done in a future refactoring on InteractinTracker to test every specific module.

draffensperger · 2019-06-27T17:00:41Z

packages/opencensus-web-instrumentation-zone/src/interaction-tracker.ts

-  }
+  // Don't count periodic tasks like setInterval as they will be repeatedly
+  // called.
+  if (task.data.isPeriodic) return false;


Why did you change this to ignore all periodic tasks? I would think that calls to setInterval could be pretty relevant to the work of user interactions.

One other thing that should be a TODO for later I think - if you have a periodic task with > 1 second of time, then I think it should be executed outside of the tracing zone. Otherwise, if it ends up doing some work in the future it will still be associated with that same older tracing zone.

I found out that if we track periodic tasks, the interaction might not finish as they are called repeatedly, so we won't be able to determine the stability of the interaction.

packages/opencensus-web-instrumentation-zone/test/test-interaction-tracker.ts

draffensperger · 2019-06-27T17:03:17Z

packages/opencensus-web-instrumentation-zone/test/test-interaction-tracker.ts

+      // As there is another setTimeOut that completes the interaction, the
+      // span duraction is not precise, then only test if the interaction duration
+      // finishes within a range.
+      expect(rootSpan.duration).toBeLessThan(5);


Should you check a minimum time here too?

In this case, this noop sync function does not have any logic.Then, the minimum time would be 0. I think that just checking the max time is fine.

packages/opencensus-web-instrumentation-zone/test/test-interaction-tracker.ts

draffensperger · 2019-06-27T17:18:32Z

packages/opencensus-web-instrumentation-zone/test/test-on-page-interaction.ts

+      spyOn(tracer, 'onEndSpan');
+      interaction.stopAndRecord();
+
+      expect(root.attributes['EventType']).toBe('click');


Should the test cases above for the fake user interactions also test these attributes?

Thanks, Added.

codecov-io · 2019-06-28T14:50:40Z

Codecov Report

Merging #104 into master will decrease coverage by 0.29%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #104     +/-   ##
=======================================
- Coverage   96.29%    96%   -0.3%     
=======================================
  Files          26     28      +2     
  Lines         647    825    +178     
  Branches       83    124     +41     
=======================================
+ Hits          623    792    +169     
- Misses         24     33      +9

Impacted Files	Coverage Δ
src/interaction-tracker.ts	`93.54% <0%> (ø)`
src/on-page-interaction-stop-watch.ts	`100% <0%> (ø)`
src/trace/model/tracing.ts	`100% <0%> (+4.54%)`	⬆️

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 a0d573f...6a3b59e. Read the comment docs.

draffensperger · 2019-06-28T16:17:49Z

packages/opencensus-web-core/src/trace/model/tracing.ts

+  constructor() {
+    // register the noop exporter to have an span event listener,
+    // helpful for testing.
+    this.tracer.registerSpanEventListener(this.exporter);


Could you do this in the tests rather than here in the production code?

draffensperger · 2019-06-28T16:20:13Z

packages/opencensus-web-instrumentation-zone/test/test-on-page-interaction.ts

+import { RootSpan, Tracer } from '@opencensus/web-core';
+import { OnPageInteractionStopwatch } from '../src/on-page-interaction';
+
+describe('OnPageInteraction', () => {


Should this be 'OnPageInteractionStopwatch'? And should the file be something like test-on-page-interaction-stopwatch.ts?

While preparing for plugins, in a piecemeal fashion firstly added some interfaces and a guide but since the plugins proposal and implementation didn't make it at the last minute, let's also remove the previous code to avoid confusion. Fixes census-instrumentation#104

Add testing for interaction tracker

41db4ef

crdgonzalezca requested review from draffensperger, johnbryan and mayurkale22 as code owners June 27, 2019 15:08

draffensperger reviewed Jun 27, 2019

View reviewed changes

crdgonzalezca added 2 commits June 28, 2019 10:20

Refactoring on testing and interaction tracker

be618e5

Update tracing testing

f7783ef

draffensperger suggested changes Jun 28, 2019

View reviewed changes

Few changes and refactoring

6a3b59e

draffensperger approved these changes Jun 28, 2019

View reviewed changes

draffensperger merged commit 1b3a192 into census-instrumentation:master Jun 28, 2019

crdgonzalezca deleted the testing branch July 24, 2019 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit/integration testing for @opencensus/web-instrumentation-zone #104

Add unit/integration testing for @opencensus/web-instrumentation-zone #104

crdgonzalezca commented Jun 27, 2019 •

edited

draffensperger Jun 27, 2019

crdgonzalezca Jun 27, 2019

crdgonzalezca Jun 28, 2019

draffensperger Jun 27, 2019

crdgonzalezca Jun 27, 2019

draffensperger Jun 27, 2019

crdgonzalezca Jun 28, 2019

draffensperger Jun 27, 2019

crdgonzalezca Jun 27, 2019

draffensperger Jun 27, 2019

crdgonzalezca Jun 28, 2019

draffensperger Jun 27, 2019

crdgonzalezca Jun 28, 2019

codecov-io commented Jun 28, 2019 •

edited

draffensperger Jun 28, 2019

crdgonzalezca Jun 28, 2019

draffensperger Jun 28, 2019

crdgonzalezca Jun 28, 2019

Add unit/integration testing for @opencensus/web-instrumentation-zone #104

Add unit/integration testing for @opencensus/web-instrumentation-zone #104

Conversation

crdgonzalezca commented Jun 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 28, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crdgonzalezca commented Jun 27, 2019 •

edited

codecov-io commented Jun 28, 2019 •

edited