From 9c8898f4c3138e1c65bfc87a3e320dd4ed44db10 Mon Sep 17 00:00:00 2001 From: Nikolaos Achilles Date: Thu, 15 Jul 2021 17:45:39 +0300 Subject: [PATCH 1/6] fix: headers are appended to existing one (open-telemetry#2335) --- .../src/platform/browser/util.ts | 7 ++- .../test/browser/util.test.ts | 63 +++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 packages/opentelemetry-exporter-collector/test/browser/util.test.ts diff --git a/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts b/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts index a695ee6944..f085d7ae6a 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts @@ -55,8 +55,11 @@ export function sendWithXhr( ) { const xhr = new XMLHttpRequest(); xhr.open('POST', url); - xhr.setRequestHeader('Accept', 'application/json'); - xhr.setRequestHeader('Content-Type', 'application/json'); + + if (!Object.keys(headers).includes('Accept')) + xhr.setRequestHeader('Accept', 'application/json'); + if (!Object.keys(headers).includes('Content-Type')) + xhr.setRequestHeader('Content-Type', 'application/json'); Object.entries(headers).forEach(([k, v]) => { xhr.setRequestHeader(k, v); }); diff --git a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts new file mode 100644 index 0000000000..1e383613da --- /dev/null +++ b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts @@ -0,0 +1,63 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as sinon from 'sinon'; +import { sendWithXhr } from "../../src/platform/browser/util"; +import { + ensureHeadersContain, +} from '../helper'; + + +describe('util - browser', () => { + + describe('when Content-Type and Accept headers are set explicit', () => { + let server: any; + + const explicitContentTypeAndAcceptHeaders = { + 'Content-Type': 'application/json', + 'Accept': 'application/json', + }; + const body = ""; + const url = ""; + + let onSuccessStub: sinon.SinonStub; + let onErrorStub: sinon.SinonStub; + + beforeEach(() => { + onSuccessStub = sinon.stub(); + onErrorStub = sinon.stub(); + server = sinon.fakeServer.create(); + }); + afterEach(() => { + server.restore(); + sinon.restore(); + }); + + it('should successfully use XMLHttpRequest and Request Headers contain the expilicit defined Content-Type and Accept Header Values', done => { + sendWithXhr(body, url, explicitContentTypeAndAcceptHeaders, onSuccessStub, onErrorStub) + + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { ...explicitContentTypeAndAcceptHeaders, "Content-Type": "application/json;charset=utf-8" } + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); + }); + }); +}); + From 20615812d299a923eae279abbaa4c7d5cebae332 Mon Sep 17 00:00:00 2001 From: Nikolaos Achilles Date: Fri, 16 Jul 2021 13:59:53 +0300 Subject: [PATCH 2/6] test(browser-util): added 2 more test-cases refactored desc, it wording --- .../test/browser/util.test.ts | 94 ++++++++++++++----- 1 file changed, 70 insertions(+), 24 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts index 1e383613da..0777a99e1e 100644 --- a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts +++ b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts @@ -14,43 +14,90 @@ * limitations under the License. */ -import * as sinon from 'sinon'; +import * as sinon from "sinon"; import { sendWithXhr } from "../../src/platform/browser/util"; -import { - ensureHeadersContain, -} from '../helper'; +import { ensureHeadersContain } from "../helper"; +describe("util - browser", () => { + let server: any; + const body = ""; + const url = ""; -describe('util - browser', () => { + let onSuccessStub: sinon.SinonStub; + let onErrorStub: sinon.SinonStub; - describe('when Content-Type and Accept headers are set explicit', () => { - let server: any; + beforeEach(() => { + onSuccessStub = sinon.stub(); + onErrorStub = sinon.stub(); + server = sinon.fakeServer.create(); + }); + + afterEach(() => { + server.restore(); + sinon.restore(); + }); + describe("when Content-Type and Accept headers are set explicit", () => { const explicitContentTypeAndAcceptHeaders = { - 'Content-Type': 'application/json', - 'Accept': 'application/json', + "Content-Type": "application/json", + Accept: "application/json", }; - const body = ""; - const url = ""; + it("should successfully use XMLHttpRequest, Request Headers contain the expilicit headers", done => { - let onSuccessStub: sinon.SinonStub; - let onErrorStub: sinon.SinonStub; + sendWithXhr( + body, + url, + explicitContentTypeAndAcceptHeaders, + onSuccessStub, + onErrorStub + ); - beforeEach(() => { - onSuccessStub = sinon.stub(); - onErrorStub = sinon.stub(); - server = sinon.fakeServer.create(); + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { + ...explicitContentTypeAndAcceptHeaders, + "Content-Type": "application/json;charset=utf-8", + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); }); - afterEach(() => { - server.restore(); - sinon.restore(); + }); + + describe("when headers are set empty {}", () => { + const emptyHeaders = {}; + it('should successfully use XMLHttpRequest, Request Headers contain Content-Type of value "application/json" and Accept of value "application/json"', done => { + + sendWithXhr(body, url, emptyHeaders, onSuccessStub, onErrorStub); + + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { + Accept: "application/json", + "Content-Type": "application/json;charset=utf-8", + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); }); + }); + + describe("when custom headers are set", () => { + const customHeaders = { aHeader: "aValue", bHeader: "bValue" }; + it('should successfully use XMLHttpRequest, Request Headers contain Content-Type of value "application/json", Accept of value "application/json" and custom headers', done => { - it('should successfully use XMLHttpRequest and Request Headers contain the expilicit defined Content-Type and Accept Header Values', done => { - sendWithXhr(body, url, explicitContentTypeAndAcceptHeaders, onSuccessStub, onErrorStub) + sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { ...explicitContentTypeAndAcceptHeaders, "Content-Type": "application/json;charset=utf-8" } + const expectedHeaders = { + ...customHeaders, + Accept: "application/json", + "Content-Type": "application/json;charset=utf-8", + }; setTimeout(() => { const { requestHeaders } = server.requests[0]; @@ -60,4 +107,3 @@ describe('util - browser', () => { }); }); }); - From ae0b2761c4fafb60e42ef8c4eebdd7600c92e844 Mon Sep 17 00:00:00 2001 From: Nikolaos Achilles Date: Fri, 16 Jul 2021 19:49:39 +0300 Subject: [PATCH 3/6] test: split tests review --- .../test/browser/util.test.ts | 164 +++++++++++------- 1 file changed, 103 insertions(+), 61 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts index 0777a99e1e..65dc3d6e49 100644 --- a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts +++ b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts @@ -18,7 +18,7 @@ import * as sinon from "sinon"; import { sendWithXhr } from "../../src/platform/browser/util"; import { ensureHeadersContain } from "../helper"; -describe("util - browser", () => { +describe('util - browser', () => { let server: any; const body = ""; const url = ""; @@ -37,72 +37,114 @@ describe("util - browser", () => { sinon.restore(); }); - describe("when Content-Type and Accept headers are set explicit", () => { - const explicitContentTypeAndAcceptHeaders = { - "Content-Type": "application/json", - Accept: "application/json", - }; - it("should successfully use XMLHttpRequest, Request Headers contain the expilicit headers", done => { - - sendWithXhr( - body, - url, - explicitContentTypeAndAcceptHeaders, - onSuccessStub, - onErrorStub - ); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - ...explicitContentTypeAndAcceptHeaders, - "Content-Type": "application/json;charset=utf-8", + describe('when XMLHTTPRequest is used', () => { + describe('and Content-Type header is set', () => { + const explicitContentType = { + 'Content-Type': 'application/json', }; - - setTimeout(() => { - const { requestHeaders } = server.requests[0]; - ensureHeadersContain(requestHeaders, expectedHeaders); - done(); + it('Request Headers should contain "Content-Type" header', done => { + sendWithXhr(body, url, explicitContentType, onSuccessStub, onErrorStub); + + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { + 'Content-Type': 'application/json;charset=utf-8', + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); + }); + it('Request Headers should contain "Accept" header', done => { + sendWithXhr(body, url, explicitContentType, onSuccessStub, onErrorStub); + + const expectedHeaders = { + 'Accept': 'application/json', + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); }); }); - }); - - describe("when headers are set empty {}", () => { - const emptyHeaders = {}; - it('should successfully use XMLHttpRequest, Request Headers contain Content-Type of value "application/json" and Accept of value "application/json"', done => { - sendWithXhr(body, url, emptyHeaders, onSuccessStub, onErrorStub); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - Accept: "application/json", - "Content-Type": "application/json;charset=utf-8", - }; - - setTimeout(() => { - const { requestHeaders } = server.requests[0]; - ensureHeadersContain(requestHeaders, expectedHeaders); - done(); + describe('and empty headers are set', () => { + const emptyHeaders = {}; + it('Request Headers should contain "Content-Type" header', done => { + sendWithXhr(body, url, emptyHeaders, onSuccessStub, onErrorStub); + + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { + 'Content-Type': 'application/json;charset=utf-8', + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); + }); + it('Request Headers should contain "Accept" header', done => { + sendWithXhr(body, url, emptyHeaders, onSuccessStub, onErrorStub); + + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { + 'Accept': 'application/json', + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); }); }); - }); - - describe("when custom headers are set", () => { - const customHeaders = { aHeader: "aValue", bHeader: "bValue" }; - it('should successfully use XMLHttpRequest, Request Headers contain Content-Type of value "application/json", Accept of value "application/json" and custom headers', done => { - - sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - ...customHeaders, - Accept: "application/json", - "Content-Type": "application/json;charset=utf-8", - }; - - setTimeout(() => { - const { requestHeaders } = server.requests[0]; - ensureHeadersContain(requestHeaders, expectedHeaders); - done(); + describe('and custom headers are set', () => { + const customHeaders = { aHeader: "aValue", bHeader: "bValue" }; + it('Request Headers should contain "Content-Type" header', done => { + sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); + + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { + 'Content-Type': 'application/json;charset=utf-8', + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); + }); + it('Request Headers should contain "Accept" header', done => { + sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); + + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { + 'Accept': 'application/json', + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); + }); + it('Request Headers should contain custom headers', done => { + sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); + + // ;charset=utf-8 is applied by sinon.fakeServer + const expectedHeaders = { + ...customHeaders, + }; + + setTimeout(() => { + const { requestHeaders } = server.requests[0]; + ensureHeadersContain(requestHeaders, expectedHeaders); + done(); + }); }); }); }); From 05321473b72d359a231fddee1fab5399b9e98aa6 Mon Sep 17 00:00:00 2001 From: Nikolaos Achilles Date: Fri, 16 Jul 2021 20:00:22 +0300 Subject: [PATCH 4/6] fix: review headers are appended to existing one (open-telemetry#2335) --- .../src/platform/browser/util.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts b/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts index f085d7ae6a..5c57517235 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts @@ -56,11 +56,15 @@ export function sendWithXhr( const xhr = new XMLHttpRequest(); xhr.open('POST', url); - if (!Object.keys(headers).includes('Accept')) - xhr.setRequestHeader('Accept', 'application/json'); - if (!Object.keys(headers).includes('Content-Type')) - xhr.setRequestHeader('Content-Type', 'application/json'); - Object.entries(headers).forEach(([k, v]) => { + const defaultHeaders = { + 'Accept': 'application/json', + 'Content-Type': 'application/json', + }; + + Object.entries({ + ...defaultHeaders, + ...headers, + }).forEach(([k, v]) => { xhr.setRequestHeader(k, v); }); From 0c685149a8b921b5a1a47be0f0d383c81bcdc644 Mon Sep 17 00:00:00 2001 From: Nikolaos Achilles Date: Fri, 16 Jul 2021 21:16:40 +0300 Subject: [PATCH 5/6] test: review DRY --- .../test/browser/util.test.ts | 67 ++++++------------- 1 file changed, 22 insertions(+), 45 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts index 65dc3d6e49..fc1816a58f 100644 --- a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts +++ b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts @@ -38,17 +38,22 @@ describe('util - browser', () => { }); describe('when XMLHTTPRequest is used', () => { + let expectedHeaders: Record; + beforeEach(()=>{ + expectedHeaders = { + // ;charset=utf-8 is applied by sinon.fakeServer + 'Content-Type': 'application/json;charset=utf-8', + 'Accept': 'application/json', + } + }); describe('and Content-Type header is set', () => { - const explicitContentType = { + beforeEach(()=>{ + const explicitContentType = { 'Content-Type': 'application/json', }; - it('Request Headers should contain "Content-Type" header', done => { sendWithXhr(body, url, explicitContentType, onSuccessStub, onErrorStub); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - 'Content-Type': 'application/json;charset=utf-8', - }; + }); + it('Request Headers should contain "Content-Type" header', done => { setTimeout(() => { const { requestHeaders } = server.requests[0]; @@ -57,11 +62,6 @@ describe('util - browser', () => { }); }); it('Request Headers should contain "Accept" header', done => { - sendWithXhr(body, url, explicitContentType, onSuccessStub, onErrorStub); - - const expectedHeaders = { - 'Accept': 'application/json', - }; setTimeout(() => { const { requestHeaders } = server.requests[0]; @@ -72,14 +72,11 @@ describe('util - browser', () => { }); describe('and empty headers are set', () => { - const emptyHeaders = {}; - it('Request Headers should contain "Content-Type" header', done => { + beforeEach(()=>{ + const emptyHeaders = {}; sendWithXhr(body, url, emptyHeaders, onSuccessStub, onErrorStub); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - 'Content-Type': 'application/json;charset=utf-8', - }; + }); + it('Request Headers should contain "Content-Type" header', done => { setTimeout(() => { const { requestHeaders } = server.requests[0]; @@ -88,12 +85,6 @@ describe('util - browser', () => { }); }); it('Request Headers should contain "Accept" header', done => { - sendWithXhr(body, url, emptyHeaders, onSuccessStub, onErrorStub); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - 'Accept': 'application/json', - }; setTimeout(() => { const { requestHeaders } = server.requests[0]; @@ -103,14 +94,12 @@ describe('util - browser', () => { }); }); describe('and custom headers are set', () => { - const customHeaders = { aHeader: "aValue", bHeader: "bValue" }; - it('Request Headers should contain "Content-Type" header', done => { + let customHeaders: Record; + beforeEach(()=>{ + customHeaders = { aHeader: "aValue", bHeader: "bValue" }; sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - 'Content-Type': 'application/json;charset=utf-8', - }; + }); + it('Request Headers should contain "Content-Type" header', done => { setTimeout(() => { const { requestHeaders } = server.requests[0]; @@ -119,12 +108,6 @@ describe('util - browser', () => { }); }); it('Request Headers should contain "Accept" header', done => { - sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - 'Accept': 'application/json', - }; setTimeout(() => { const { requestHeaders } = server.requests[0]; @@ -133,16 +116,10 @@ describe('util - browser', () => { }); }); it('Request Headers should contain custom headers', done => { - sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); - - // ;charset=utf-8 is applied by sinon.fakeServer - const expectedHeaders = { - ...customHeaders, - }; setTimeout(() => { const { requestHeaders } = server.requests[0]; - ensureHeadersContain(requestHeaders, expectedHeaders); + ensureHeadersContain(requestHeaders, customHeaders); done(); }); }); From 9217290832f5d34466edb47b571a8e476ab3c708 Mon Sep 17 00:00:00 2001 From: Nikolaos Achilles Date: Wed, 21 Jul 2021 18:04:14 +0300 Subject: [PATCH 6/6] fix: review lint --- .../test/browser/util.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts index fc1816a58f..ecb6c4c801 100644 --- a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts +++ b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts @@ -14,14 +14,14 @@ * limitations under the License. */ -import * as sinon from "sinon"; -import { sendWithXhr } from "../../src/platform/browser/util"; -import { ensureHeadersContain } from "../helper"; +import * as sinon from 'sinon'; +import { sendWithXhr } from '../../src/platform/browser/util'; +import { ensureHeadersContain } from '../helper'; describe('util - browser', () => { let server: any; - const body = ""; - const url = ""; + const body = ''; + const url = ''; let onSuccessStub: sinon.SinonStub; let onErrorStub: sinon.SinonStub; @@ -96,7 +96,7 @@ describe('util - browser', () => { describe('and custom headers are set', () => { let customHeaders: Record; beforeEach(()=>{ - customHeaders = { aHeader: "aValue", bHeader: "bValue" }; + customHeaders = { aHeader: 'aValue', bHeader: 'bValue' }; sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub); }); it('Request Headers should contain "Content-Type" header', done => {