-
Notifications
You must be signed in to change notification settings - Fork 18
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
nightwatch test-observability integration phase-1 #5
Conversation
src/utils/helper.js
Outdated
console.log(`Getting ${module} from ${GLOBAL_MODULE_PATH}`); | ||
|
||
const global_path = path.join(GLOBAL_MODULE_PATH, module); | ||
if (!fs.existsSync(global_path)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sure if looking for nightwatch in the global path is a good idea.
@gravityvi thoughts?
nightwatch/globals.js
Outdated
} | ||
} | ||
} catch (error) { | ||
console.log(`nightwatch-browserstack-plugin: Something went wrong in processing report file for test observability - ${error}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a helper function for logs.
src/testObservability.js
Outdated
await this.sendTestRunEvent(eventData, testFileReport, 'HookRunStarted', globalBeforeEachHookId, 'GLOBAL_BEFORE_EACH', sectionName); | ||
if (eventData.httpOutput && eventData.httpOutput.length > 0) { | ||
for (let i=0; i<eventData.httpOutput.length; i+=2) { | ||
await this.createHttpLogEvent(eventData.httpOutput[i], eventData.httpOutput[i+1], globalBeforeEachHookId); | ||
} | ||
} | ||
await this.sendTestRunEvent(eventData, testFileReport, 'HookRunFinished', globalBeforeEachHookId, 'GLOBAL_BEFORE_EACH', sectionName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this section is repeated again and again, extract is out into a function
src/utils/helper.js
Outdated
let data = eventData; | ||
let event_api_url = 'api/v1/event'; | ||
|
||
exports.requestQueueHandler.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is does not make sense.
requestQueueHandler seems to be a singleton, this state management should be in requestQueueHandler.js
itself.
src/utils/crashReporter.js
Outdated
static filterPII(settings) { | ||
const configWithoutPII = JSON.parse(JSON.stringify(settings)); | ||
if (configWithoutPII['@nightwatch/browserstack'] && configWithoutPII['@nightwatch/browserstack'].test_observability) { | ||
this.deletePIIKeysFromObject(configWithoutPII['@nightwatch/browserstack'].test_observability); | ||
} | ||
if (configWithoutPII.desiredCapabilities && configWithoutPII.desiredCapabilities['bstack:options']) { | ||
this.deletePIIKeysFromObject(configWithoutPII.desiredCapabilities['bstack:options']); | ||
} | ||
return configWithoutPII; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deletePIIKeysFromObject
is defined as static
and only being used in filterPII
, so we can include the delete functionality in filterPII
itself.
static filterPII(settings) { | |
const configWithoutPII = JSON.parse(JSON.stringify(settings)); | |
if (configWithoutPII['@nightwatch/browserstack'] && configWithoutPII['@nightwatch/browserstack'].test_observability) { | |
this.deletePIIKeysFromObject(configWithoutPII['@nightwatch/browserstack'].test_observability); | |
} | |
if (configWithoutPII.desiredCapabilities && configWithoutPII.desiredCapabilities['bstack:options']) { | |
this.deletePIIKeysFromObject(configWithoutPII.desiredCapabilities['bstack:options']); | |
} | |
return configWithoutPII; | |
} | |
static filterPII(settings) { | |
const keysToDelete = ['user', 'username', 'userName', 'key', 'accessKey']; | |
const configWithoutPII = JSON.parse(JSON.stringify(settings)); | |
const deleteKeys = (obj) => { | |
if (!obj) { | |
return; | |
} | |
keysToDelete.forEach(key => delete obj[key]); | |
} | |
if (configWithoutPII['@nightwatch/browserstack'] && configWithoutPII['@nightwatch/browserstack'].test_observability) { | |
deleteKeys(configWithoutPII['@nightwatch/browserstack'].test_observability); | |
} | |
if (configWithoutPII.desiredCapabilities && configWithoutPII.desiredCapabilities['bstack:options']) { | |
deleteKeys(configWithoutPII.desiredCapabilities['bstack:options']); | |
} | |
return configWithoutPII; | |
} |
src/utils/helper.js
Outdated
const httpKeepAliveAgent = new http.Agent({ | ||
keepAlive: true, | ||
timeout: 60000, | ||
maxSockets: 2, | ||
maxTotalSockets: 2 | ||
}); | ||
|
||
const httpsKeepAliveAgent = new https.Agent({ | ||
keepAlive: true, | ||
timeout: 60000, | ||
maxSockets: 2, | ||
maxTotalSockets: 2 | ||
}); | ||
|
||
const httpScreenshotsKeepAliveAgent = new http.Agent({ | ||
keepAlive: true, | ||
timeout: 60000, | ||
maxSockets: 2, | ||
maxTotalSockets: 2 | ||
}); | ||
|
||
const httpsScreenshotsKeepAliveAgent = new https.Agent({ | ||
keepAlive: true, | ||
timeout: 60000, | ||
maxSockets: 2, | ||
maxTotalSockets: 2 | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be consolidated into a function, which will be much cleaner
function createKeepAliveAgent(protocol) {
return new protocol.Agent({
keepAlive: true,
timeout: 60000,
maxSockets: 2,
maxTotalSockets: 2
});
}
const httpKeepAliveAgent = createKeepAliveAgent(http);
const httpsKeepAliveAgent = createKeepAliveAgent(https);
const httpScreenshotsKeepAliveAgent = createKeepAliveAgent(http);
const httpsScreenshotsKeepAliveAgent = createKeepAliveAgent(https);
nightwatch/globals.js
Outdated
delete modulesWithEnv[testSetting][testFile].completed[completedSection].testcases; | ||
} | ||
} | ||
promises.push(testObservability.processTestReportFile(JSON.parse(JSON.stringify(modulesWithEnv[testSetting][testFile])))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON.parse
and JSON.stringify
is unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deep copy is necessary because the git modules are changing the values in the object while parsing.
}; | ||
|
||
exports.shutDownRequestHandler = async () => { | ||
await requestQueueHandler.shutdown(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methods related to requesQueuetHandler should be moved to requestQueueHandler.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this method to remove circular dependency and made requesQueuetHandler as singleton
requestQueueHandler.start(); | ||
const { | ||
shouldProceed, | ||
proceedWithData, | ||
proceedWithUrl | ||
} = requestQueueHandler.add(eventData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methods related to requesQueuetHandler should be moved to requestQueueHandler.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swrdfish these are just the method calls, which can be called anywhere in the code. Can you suggest how can this be moved inside the class itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire uploadEventData
should be moved to requestQueueHandler.
src/utils/helper.js
Outdated
agent: API_URL.includes('https') ? httpsKeepAliveAgent : httpKeepAliveAgent | ||
}}; | ||
|
||
if (url === requestQueueHandler.screenshotEventUrl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methods related to requesQueuetHandler should be moved to requestQueueHandler.js
const httpScreenshotsKeepAliveAgent = createKeepAliveAgent(http); | ||
const httpsScreenshotsKeepAliveAgent = createKeepAliveAgent(https); | ||
|
||
const requestQueueHandler = require('./requestQueueHandler'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methods related to requesQueuetHandler should be moved to requestQueueHandler.js
src/utils/helper.js
Outdated
['HookRunFinished']: 'Hook_End_Upload' | ||
}[eventData.event_type]; | ||
|
||
if (process.env.BS_TESTOPS_JWT !== 'null') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if process.env.BS_TESTOPS_JWT
is undefined
?
nightwatch/globals.js
Outdated
reporter: function(results, done) { | ||
if (helper.isTestObservabilitySession()) { | ||
const promises = []; | ||
try { | ||
const modulesWithEnv = results['modulesWithEnv']; | ||
for (const testSetting in modulesWithEnv) { | ||
for (const testFile in modulesWithEnv[testSetting]) { | ||
for (const completedSection in modulesWithEnv[testSetting][testFile].completed) { | ||
if (modulesWithEnv[testSetting][testFile].completed[completedSection]) { | ||
delete modulesWithEnv[testSetting][testFile].completed[completedSection].steps; | ||
delete modulesWithEnv[testSetting][testFile].completed[completedSection].testcases; | ||
} | ||
} | ||
promises.push(testObservability.processTestReportFile(JSON.parse(JSON.stringify(modulesWithEnv[testSetting][testFile])))); | ||
} | ||
} | ||
|
||
Promise.all(promises).then(() => { | ||
done(); | ||
}).catch((err) =>{ | ||
Logger.error(`Something went wrong in processing report file for test observability - ${err.message} with stacktrace ${err.stack}`); | ||
CrashReporter.uploadCrashReport(err.message, err.stack); | ||
done(); | ||
}); | ||
|
||
return; | ||
} catch (error) { | ||
CrashReporter.uploadCrashReport(error.message, error.stack); | ||
Logger.error(`Something went wrong in processing report file for test observability - ${error.message} with stacktrace ${error.stack}`); | ||
} | ||
} | ||
done(results); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole code can be refactored like this
reporter: async function(results, done) {
if (!helper.isTestObservabilitySession()) {
done(results);
return;
}
try {
const modulesWithEnv = results['modulesWithEnv'];
const promises = [];
for (const testSetting in modulesWithEnv) {
for (const testFile in modulesWithEnv[testSetting]) {
const completedSections = modulesWithEnv[testSetting][testFile].completed;
for (const completedSection in completedSections) {
if (completedSections[completedSection]) {
delete completedSections[completedSection].steps;
delete completedSections[completedSection].testcases;
}
}
// Maybe create a helper method to do `.parse` and `.stringify`
promises.push(testObservability.processTestReportFile(JSON.parse(JSON.stringify(modulesWithEnv[testSetting][testFile]))));
}
}
await Promise.all(promises);
done();
} catch (error) {
Logger.error(`Something went wrong in processing report file for test observability - ${error.message} with stacktrace ${error.stack}`);
CrashReporter.uploadCrashReport(error.message, error.stack);
}
}
src/testObservability.js
Outdated
name: helper.getObservabilityBuild(this._settings, this._bstackOptions), | ||
build_identifier: options.buildIdentifier, | ||
description: options.buildDescription || '', | ||
start_time: (new Date()).toISOString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start_time: (new Date()).toISOString(), | |
start_time: new Date().toISOString(), |
src/testObservability.js
Outdated
if (response.data && response.data.jwt) { | ||
process.env.BS_TESTOPS_JWT = response.data.jwt; | ||
} | ||
if (response.data && response.data.build_hashed_id) { | ||
process.env.BS_TESTOPS_BUILD_HASHED_ID = response.data.build_hashed_id; | ||
} | ||
if (response.data && response.data.allow_screenshots) { | ||
process.env.BS_TESTOPS_ALLOW_SCREENSHOTS = response.data.allow_screenshots.toString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (response.data && response.data.jwt) { | |
process.env.BS_TESTOPS_JWT = response.data.jwt; | |
} | |
if (response.data && response.data.build_hashed_id) { | |
process.env.BS_TESTOPS_BUILD_HASHED_ID = response.data.build_hashed_id; | |
} | |
if (response.data && response.data.allow_screenshots) { | |
process.env.BS_TESTOPS_ALLOW_SCREENSHOTS = response.data.allow_screenshots.toString(); | |
} | |
const responseData = response.data || {}; | |
if (responseData.jwt) { | |
process.env.BS_TESTOPS_JWT = responseData.jwt; | |
} | |
if (responseData.build_hashed_id) { | |
process.env.BS_TESTOPS_BUILD_HASHED_ID = responseData.build_hashed_id; | |
} | |
if (responseData.allow_screenshots) { | |
process.env.BS_TESTOPS_ALLOW_SCREENSHOTS = responseData.allow_screenshots.toString(); | |
} |
src/testObservability.js
Outdated
await helper.shutDownRequestHandler(); | ||
try { | ||
const response = await helper.makeRequest('PUT', `api/v1/builds/${process.env.BS_TESTOPS_BUILD_HASHED_ID}/stop`, data, config); | ||
if (response.data && response.data.error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (response.data && response.data.error) { | |
if (response.data?.error) { |
src/testObservability.js
Outdated
}; | ||
} | ||
const data = { | ||
'stop_time': (new Date()).toISOString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'stop_time': (new Date()).toISOString() | |
'stop_time': new Date().toISOString() |
src/testObservability.js
Outdated
try { | ||
const response = await helper.makeRequest('PUT', `api/v1/builds/${process.env.BS_TESTOPS_BUILD_HASHED_ID}/stop`, data, config); | ||
if (response.data && response.data.error) { | ||
throw ({message: response.data.error}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw ({message: response.data.error}); | |
throw {message: response.data.error}; |
src/testObservability.js
Outdated
break; | ||
} | ||
default: { | ||
if (eventData.retryTestData && eventData.retryTestData.length>0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (eventData.retryTestData && eventData.retryTestData.length>0) { | |
if (eventData.retryTestData?.length) { |
src/testObservability.js
Outdated
} | ||
} | ||
} | ||
if (skippedTests && skippedTests.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (skippedTests && skippedTests.length > 0) { | |
if (skippedTests?.length > 0) { |
src/testObservability.js
Outdated
|
||
async processTestRunData (eventData, sectionName, testFileReport, hookIds) { | ||
const testUuid = uuidv4(); | ||
const errorData = eventData.commands.find(command => command.result && command.result.stack); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const errorData = eventData.commands.find(command => command.result && command.result.stack); | |
const errorData = eventData.commands.find(command => command.result?.stack); |
src/testObservability.js
Outdated
if (eventData.httpOutput && eventData.httpOutput.length > 0) { | ||
for (let i=0; i<eventData.httpOutput.length; i+=2) { | ||
await this.createHttpLogEvent(eventData.httpOutput[i], eventData.httpOutput[i+1], testUuid); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (eventData.httpOutput && eventData.httpOutput.length > 0) { | |
for (let i=0; i<eventData.httpOutput.length; i+=2) { | |
await this.createHttpLogEvent(eventData.httpOutput[i], eventData.httpOutput[i+1], testUuid); | |
} | |
} | |
for (const [index, output] of eventData.httpOutput.entries()) { | |
if (index % 2 === 0) { | |
await this.createHttpLogEvent(output, eventData.httpOutput[index + 1], testUuid); | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a small refactor suggestion, but overall I think this PR is looking great!
src/utils/requestHelper.js
Outdated
exports.makeRequest = (type, url, data, config) => { | ||
|
||
return new Promise((resolve, reject) => { | ||
const options = {...config, ...{ | ||
method: type, | ||
url: `${API_URL}/${url}`, | ||
body: data, | ||
json: config.headers['Content-Type'] === 'application/json', | ||
agent: API_URL.includes('https') ? httpsKeepAliveAgent : httpKeepAliveAgent | ||
}}; | ||
|
||
if (url === SCREENSHOT_EVENT_URL) { | ||
options.agent = API_URL.includes('https') ? httpsScreenshotsKeepAliveAgent : httpScreenshotsKeepAliveAgent; | ||
} | ||
|
||
request(options, function callback(error, response, body) { | ||
if (error) { | ||
reject(error); | ||
} else if (response.statusCode !== 200) { | ||
if (response.statusCode === 401) { | ||
reject(response && response.body ? response.body : `Received response from BrowserStack Server with status : ${response.statusCode}`); | ||
} else { | ||
reject(`Received response from BrowserStack Server with status : ${response.statusCode}`); | ||
} | ||
} else { | ||
try { | ||
if (body && typeof(body) !== 'object') {body = JSON.parse(body)} | ||
} catch (e) { | ||
reject('Not a JSON response from BrowserStack Server'); | ||
} | ||
resolve({ | ||
data: body | ||
}); | ||
} | ||
}); | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I had suggested small refactor
exports.makeRequest = (type, url, data, config) => { | |
return new Promise((resolve, reject) => { | |
const options = {...config, ...{ | |
method: type, | |
url: `${API_URL}/${url}`, | |
body: data, | |
json: config.headers['Content-Type'] === 'application/json', | |
agent: API_URL.includes('https') ? httpsKeepAliveAgent : httpKeepAliveAgent | |
}}; | |
if (url === SCREENSHOT_EVENT_URL) { | |
options.agent = API_URL.includes('https') ? httpsScreenshotsKeepAliveAgent : httpScreenshotsKeepAliveAgent; | |
} | |
request(options, function callback(error, response, body) { | |
if (error) { | |
reject(error); | |
} else if (response.statusCode !== 200) { | |
if (response.statusCode === 401) { | |
reject(response && response.body ? response.body : `Received response from BrowserStack Server with status : ${response.statusCode}`); | |
} else { | |
reject(`Received response from BrowserStack Server with status : ${response.statusCode}`); | |
} | |
} else { | |
try { | |
if (body && typeof(body) !== 'object') {body = JSON.parse(body)} | |
} catch (e) { | |
reject('Not a JSON response from BrowserStack Server'); | |
} | |
resolve({ | |
data: body | |
}); | |
} | |
}); | |
}); | |
}; | |
exports.makeRequest = (type, url, data, config) => { | |
const isHttps = API_URL.includes('https'); | |
if (url === SCREENSHOT_EVENT_URL) { | |
agent = isHttps ? httpsScreenshotsKeepAliveAgent : httpScreenshotsKeepAliveAgent; | |
} else { | |
agent = isHttps ? httpsKeepAliveAgent : httpKeepAliveAgent; | |
} | |
const options = { | |
...config, | |
method: type, | |
url: `${API_URL}/${url}`, | |
body: data, | |
json: config.headers['Content-Type'] === 'application/json', | |
agent | |
}; | |
return new Promise((resolve, reject) => { | |
request(options, function callback(error, response, body) { | |
if (error) { | |
reject(error); | |
} else if (response.statusCode !== 200) { | |
if (response.statusCode === 401) { | |
reject(response && response.body ? response.body : `Received response from BrowserStack Server with status : ${response.statusCode}`); | |
} else { | |
reject(`Received response from BrowserStack Server with status : ${response.statusCode}`); | |
} | |
} else { | |
try { | |
if (body && typeof(body) !== 'object') {body = JSON.parse(body)} | |
} catch (e) { | |
reject('Not a JSON response from BrowserStack Server'); | |
} | |
resolve({ | |
data: body | |
}); | |
} | |
}); | |
}); | |
}; |
}; | ||
|
||
exports.getObservabilityProject = (options, bstackOptions={}) => { | ||
if (options.test_observability && options.test_observability.projectName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered using optional chaining? I've noticed it being used throughout the files.
No description provided.