Test coverage for Xml Http Plugin #2

thgao · 2020-06-15T21:44:15Z

Which problem is this PR solving?

TODO: reference issue properly when changing base to main repo

increase code coverage of web and plugin-xml-http-request packages open-telemetry/opentelemetry-js#1087

Short description of the changes

Added tests to increase code coverage in xhr.ts to at least 90%

Co-authored-by: Daniel Dyla <[email protected]>

…try#1199)

)

…telemetry#1197) Co-authored-by: Mayur Kale <[email protected]>

* chore: adding plugin-fetch and example * chore: investigating failing test * chore: chore fixing tests with better fetch mocking * chore: addressing comments * chore: lint * chore: addressing comments * chore: updating webpack-env * chore: fixes after update for node types * chore: addressing reviews * chore: fixes after merge * chore: updating version Co-authored-by: Mayur Kale <[email protected]> Co-authored-by: Daniel Dyla <[email protected]>

dane1122 · 2020-06-17T16:07:21Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

+
+        it('should NOT clear the resources', () => {
+          assert.strictEqual(
+            clearResourceTimingsSpy.args.length,


Are we testing if the clearResourceTimings function was ever called or are we testing the length of the argument passed (which is what it currently appears to be testing).
If what you wish to check for is the number of times the method was invoked, you could use assert.strictEqual(clearResourceTimingsSpy.notCalled).

dane1122 · 2020-06-17T16:39:26Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

+
+          it('should NOT create any span', () => {
+            assert.strictEqual(
+              exportSpy.args.length,


Same as above comment.

dane1122 · 2020-06-17T16:40:57Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

+
+          it('should clear the resources', () => {
+            assert.strictEqual(
+              clearResourceTimingsSpy.args.length,


Same as above. You could use notCalled, calledOnce, calledTwice to check how many times a method was called.

dane1122 · 2020-06-17T16:41:48Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

+        });
+
+        describe('when reusing the same XML Http request', () => {
+          let reusableReq: XMLHttpRequest; //= new XMLHttpRequest();


NIT: is this a left-behind comment?

dane1122 · 2020-06-17T16:43:50Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

+          let reusableReq: XMLHttpRequest; //= new XMLHttpRequest();
+          const firstUrl = 'http://localhost:8090/get';
+          const secondUrl = 'http://localhost:8099/get';
+          const getDataReuseXHR = (


Could you reuse getData? I see that the only difference is that getDataReuseXHR does not have the timeout or onAbort set.

I added getDataReuseXHR to call .open and .send on the same XMLHttpRequest object every time it's called, whereas in the original getData, a new request object is created every time the function is called.
I can change getData to take in a request object and have it continue to create a new XML request every time it's currently called

dane1122 · 2020-06-17T16:51:36Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

+          });
+
+          it('should clear previous span information', () => {
+            const span: tracing.ReadableSpan = exportSpy.args[2][0][0];


Out of curiosity, do you know why you check exportSpy.args[2][0][0] instead of exportSpy.args[1][0][0]? I am assuming you are checking for the XHR with the secondUrl.

I know to check the arg at [2] because this test is nested inside a beforeEach (which sets up other stuff like spies as well) that sets up a preflight and initial span created in addition to the span created for this test.
Would it be better to pull out this test & the other tests that rely on the other beforeEach like I did for the unsuccessful cases?

I see. Thank you for the explanation!

dane1122 · 2020-06-17T17:03:23Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

+            const attributes = span.attributes;
+            const keys = Object.keys(attributes);
+
+            assert.ok(


I see that you added various request unsuccessful cases! The test cases look good.

I noticed that the test file became a bit lengthy and am wondering if we could maybe factor out the span attribute testing into a function and call that function instead (I am unsure if this would be a good idea since we using too many helper functions is usually discouraged in test files for easier readability, so I may ask for Eryn's opinion as well).

Helper functions can be useful in tests if they don't negatively impact the readability.

It looks like enough changes in each assertion that keeping them separate is probably the better idea.

codecov-commenter · 2020-06-17T18:12:54Z

Codecov Report

❗ No coverage uploaded for pull request base (master@0596054). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master       #2   +/-   ##
=========================================
  Coverage          ?   94.11%           
=========================================
  Files             ?        2           
  Lines             ?      187           
  Branches          ?       27           
=========================================
  Hits              ?      176           
  Misses            ?       11           
  Partials          ?        0

yozora · 2020-06-17T19:45:22Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

+            const attributes = span.attributes;
+            const keys = Object.keys(attributes);
+
+            assert.ok(


Helper functions can be useful in tests if they don't negatively impact the readability.

It looks like enough changes in each assertion that keeping them separate is probably the better idea.

yozora · 2020-06-17T19:49:53Z

packages/opentelemetry-plugin-xml-http-request/test/xhr.test.ts

        let rootSpan: api.Span;
        let spyEntries: any;
-        const url = `${window.location.origin}/xml-http-request.js`;
+        const url = 'http://localhost:8090/xml-http-request.js';


Is there any danger with coding this to a specific host/port. For example, it is possible that they could be changed by configuration somewhere?

The link doesn't point to anything in the test suite, it ends up getting called with a FakeXMLHttpRequest so I don't think it would be affected
The tests written for the fetch api do this hard coding as well so I think it should be safe: https://github.com/open-telemetry/opentelemetry-js/blob/a2d1964b16018837c009f6f75351a41a00a66650/packages/opentelemetry-plugin-fetch/test/fetch.test.ts#L100

…1204) Co-authored-by: Mayur Kale <[email protected]>

* chore: fixing zone from which to fork a new zone * chore: adding tests for creating zone Co-authored-by: Mayur Kale <[email protected]>

Signed-off-by: Tina Gao <[email protected]>

added test for when request url is an ignored url from plugin config

fix header propagation tests that were affected by added a preflight span

Added tests for clearTimingResources setting in the plugin's config for the default behaviour when unspecified (false) and for explicitly setting to true.

Co-authored-by: Daniel Dyla <[email protected]> Co-authored-by: Mayur Kale <[email protected]>

…#1223)

Co-authored-by: Daniel Dyla <[email protected]>

…y#1222)

Co-authored-by: Mayur Kale <[email protected]>

renovate-bot and others added 7 commits June 15, 2020 23:54

chore(deps): update dependency sinon to v9 (open-telemetry#1196)

87882cb

Add @legendecas to approvers / CODEOWNERS (open-telemetry#1198)

56a2413

Co-authored-by: Daniel Dyla <[email protected]>

chore(deps): update typescript-eslint monorepo to v3.3.0 (open-teleme…

fd5c142

…try#1199)

chore(deps): update dependency typescript to v3.9.5 (open-telemetry#1195

b20bbf9

)

fix(exporter-collector): default endpoint for node and browser (open-…

deb3570

…telemetry#1197) Co-authored-by: Mayur Kale <[email protected]>

chore(deps): pin dependencies (open-telemetry#1205)

744c871

dane1122 reviewed Jun 17, 2020

View reviewed changes

feat: add node-plugins-all package (open-telemetry#1181)

a2d1964

yozora reviewed Jun 17, 2020

View reviewed changes

renovate-bot and others added 13 commits June 17, 2020 22:21

fix(deps): pin dependencies (open-telemetry#1212)

7afc454

feat: collector exporter custom headers and metadata (open-telemetry#…

28ad2ce

…1204) Co-authored-by: Mayur Kale <[email protected]>

chore: fixing zone from which to fork a new zone (open-telemetry#1209)

2486c3d

* chore: fixing zone from which to fork a new zone * chore: adding tests for creating zone Co-authored-by: Mayur Kale <[email protected]>

test(opentelemetry-plugin-xml-http-request): added un/patching tests

d80f37f

Signed-off-by: Tina Gao <[email protected]>

test(opentelemetry-plugin-xml-http-request): added CORS preflight tests

c7f8d33

style(opentelemetry-plugin-xml-http-request): ran lint:fix

595ee85

test(opentelemetry-plugin-xml-http-request): ignored urls

0898236

added test for when request url is an ignored url from plugin config

test(opentelemetry-plugin-xml-http-request): header prop tests

7a88ba5

fix header propagation tests that were affected by added a preflight span

test(opentelemetry-plugin-xml-http-request): clearTimingResources config

5d8c2c6

Added tests for clearTimingResources setting in the plugin's config for the default behaviour when unspecified (false) and for explicitly setting to true.

test(opentelemetry-plugin-xml-http-request): added test for XHR reuse

b9fc81a

test(opentelemetry-plugin-xml-http-request): added error handler tests

8f11a63

style(opentelemetry-plugin-xml-http-request): cleaned up

b0fedd7

refactor(opentelemetry-plugin-xml-http-request): use common getData

0ca3f9f

thgao force-pushed the test-coverage-xml-http-plugin branch from a8ef57a to 0ca3f9f Compare June 18, 2020 19:19

dyladan and others added 5 commits June 18, 2020 16:44

chore: fix markdown linting and add npm script (open-telemetry#1217)

f9f5df5

Merge branch 'master' into test-coverage-xml-http-plugin

076513d

Adds possibility to set headers to zipkin exporter (open-telemetry#1202)

2afaffa

Co-authored-by: Daniel Dyla <[email protected]> Co-authored-by: Mayur Kale <[email protected]>

chore: 0.9.0 release proposal (open-telemetry#1208)

904245f

chore(deps): update dependency webpack-cli to v3.3.12 (open-telemetry…

1f20b48

…#1223)

jonahrosenblum and others added 6 commits June 19, 2020 11:26

fix: change default propagator to match spec (open-telemetry#1218)

298b19f

Co-authored-by: Daniel Dyla <[email protected]>

chore(deps): update dependency @grpc/grpc-js to v1.1.1 (open-telemetr…

c78182f

…y#1222)

chore(deps): update dependency ts-loader to v7 (open-telemetry#1213)

f332667

chore(renovate): remove rebaseWhen config (open-telemetry#1226)

b7d08c7

fix(jsdoc): change null to undefined (open-telemetry#1231)

5f059a5

Co-authored-by: Mayur Kale <[email protected]>

Merge branch 'master' into test-coverage-xml-http-plugin

f7988a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test coverage for Xml Http Plugin #2

Test coverage for Xml Http Plugin #2

thgao commented Jun 15, 2020

dane1122 Jun 17, 2020

dane1122 Jun 17, 2020

dane1122 Jun 17, 2020

dane1122 Jun 17, 2020

dane1122 Jun 17, 2020

thgao Jun 17, 2020

dane1122 Jun 17, 2020

thgao Jun 17, 2020

dane1122 Jun 17, 2020

dane1122 Jun 17, 2020

yozora Jun 17, 2020

codecov-commenter commented Jun 17, 2020 •

edited

Loading

yozora Jun 17, 2020

yozora Jun 17, 2020

thgao Jun 17, 2020

Test coverage for Xml Http Plugin #2

Are you sure you want to change the base?

Test coverage for Xml Http Plugin #2

Conversation

thgao commented Jun 15, 2020

Which problem is this PR solving?

Short description of the changes

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-commenter commented Jun 17, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 17, 2020 •

edited

Loading