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

Rewrite axios integration tests to use v2 syntax and also execute in browser #372

Merged
merged 7 commits into from
Jun 28, 2019

Conversation

codefromthecrypt
Copy link
Member

@codefromthecrypt codefromthecrypt commented Jun 27, 2019

This extracts middleware such that axios tests can execute normally and also
in browser with Karma. It also swaps assertion style to v2 for clarity.

const ctxImpl = new ExplicitContext();
const recorder = {record};
const recorder = new BatchRecorder({logger: {logSpan: (span) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

here is how to get javascript objects that look exactly like v2 json

verifyGetSpan({
'http.path': badPath,
'http.status_code': '500',
'error': '500'
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a good example as it shows all tags succinctly are in the same span document

@@ -6,14 +6,13 @@
"scripts": {
"build": "babel src -d lib",
"test": "mocha --exit --require ../../test/helper.js",
"test-browser": "karma start --single-run --browsers ChromeHeadless ../../karma.conf.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @ewhauser now this module can pass tests in the browser, even integration tests

@@ -1,3 +1,6 @@
const path = require('path');
const testMiddleware = require('./test/middleware');
Copy link
Member Author

Choose a reason for hiding this comment

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

this sneakily mounts stub endpoints on the server that hosts the test classes used in browser testing. that way integration tests work both in and out of browser environment

@@ -16,6 +16,7 @@
"test:ts": "tsr packages/zipkin/test-types/*.test.ts --noAnnotate --libDeclarations && mocha --opts test/mocha-types.opts",
"test": "npm run test:es && npm run test:ts",
"lerna-test": "lerna run test",
"lerna-test-browser": "lerna run test-browser",
Copy link
Member Author

Choose a reason for hiding this comment

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

Ex npm run lerna-test-browser -- --scope zipkin-instrumentation-axiosjs

const path = '/weather/wuhan';
const url = () => `${baseUrl}${path}?index=10&count=300`; // defers access to baseUrl

function verifyGetSpan(tags) {
Copy link
Member Author

Choose a reason for hiding this comment

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

example full data comparison.. much better than looking at annotations, right?

@codefromthecrypt
Copy link
Member Author

ps I know build is broke btw I mainly wanted to open for feedback while I polish things..

@codefromthecrypt codefromthecrypt changed the title WIP: Rewrite axios integration tests to use v2 syntax and also execute in browser Rewrite axios integration tests to use v2 syntax and also execute in browser Jun 28, 2019
@codefromthecrypt
Copy link
Member Author

ok the build is cleaner now, and I backfilled a missing test and some missing defensive code.

Adrian Cole added 2 commits June 28, 2019 12:14
… browser

This extracts middleware such that axios tests can execute normally and also
in browser with Karma. It also swaps assertion style to v2 for clarity.
@codefromthecrypt
Copy link
Member Author

fixed merge conflicts arising from last nights release

@codefromthecrypt
Copy link
Member Author

some old firefox will look into it

FAILED TESTS:
  axios instrumentation - integration test
    ✖ should report 404 in tags
      Firefox 56.0.0 (Linux 0.0.0)
    Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

@codefromthecrypt
Copy link
Member Author

Merging to carry on and do the other http clients while in the zone. I'll take any questions/feedback post merge.

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.

1 participant