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

Upload newman runs to Postman #2849

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fixed failing unit tests for upload run
  • Loading branch information
UmedJadhav committed Sep 27, 2021
commit c299ab9f52790d68b0598bbc0b6de1d5f2849f58
218 changes: 116 additions & 102 deletions lib/run/upload-run.js
Original file line number Diff line number Diff line change
@@ -1,118 +1,132 @@
const util = require('../util');

const _ = require('lodash'),
coditva marked this conversation as resolved.
Show resolved Hide resolved
print = require('../print'),
retry = require('async-retry'),
request = require('postman-request/request'),
request = require('postman-request'),
ERROR_GO_TO_LINK = '<TBA>',
UPLOAD_RUN_API_URL = '<TBA>',
PROCESS_EXIT_SUCCESS = 0,
PROCESS_EXIT_FAIL = 1;

// Adapter to convert emitter.summary Object to Run Payload Object
// Discuss with Harsh to finalize the structure
const buildRunObject = (runSummary, collectionUID, environmentUID) => {}

/**
* Internal upload call
*
* @param {Object} uploadOptions
* @returns {function} Returns an async function which can be used by async-retry library to have retries
*/
const _upload = (uploadOptions) => {
return async(bail) => {
await new Promise((resolve, reject) => {
request(uploadOptions , (error, response, body) => {
if(error){
return reject(error); // Retry since the Error will be ERRCONNECT
}

if(200 <= response.statusCode && response.statusCode <= 299){
return resolve(body);
}

if( 400 <= response.statusCode && response.statusCode<= 499){
bail(new Error(body.message)); // Avoid retry if there is client side error ( API key error / Workspace ID / permission error)
return;
}

if( 500 <= response.statusCode && response.statusCode <= 599){ // Perform Retry if Server side Error
return reject(`Retrying because of server error: ${body.message}`);
}

return reject(); // This should not be activated ( Discuss with Harsh)
});
});
}
}
UPLOAD_RUN_API_URL = '<TBA>'

/**
* Internal upload function which handles the retry
*
* @param {*} uploadOptions
* @param {*} retryOptions
* @returns {Promise}
*/
const _uploadWithRetry = (uploadOptions, retryOptions) => {
const upload = _upload(uploadOptions)
return retry( upload,{
retries: retryOptions.maxRetries,
factor: retryOptions.retryDelayMultiplier,
randomize: retryOptions.addJitter || false,
maxRetryTime: retryOptions.maxRetryDelay * 1000 , // converting to ms
maxTimeout: retryOptions.totalTimeout * 1000 // converting to ms
});
}
class uploadRunToPostman{
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to create a class for this. Class is needed only if you want to abstract some data and use the instance in multiple places. In our case, we only need a single method.

Sidenote: The name of the class always starts with a capital letter.


/**
* Uploads Newman Run Results to Postman
*
* @param {*} uploadConfig
* @param {*} runSummary
* @returns {Number} - The exit process code . Values can be - PROCESS_EXIT_SUCCESS or PROCESS_EXIT_FAIL constants
*/
const uploadRunToPostman = async(uploadConfig, runSummary) => {

if(!uploadConfig.publishWorkspace){
return PROCESS_EXIT_SUCCESS;
constructor(uploadConfig, runSummary){
this.uploadConfig = uploadConfig,
this.runSummary = runSummary;
}

if(!uploadConfig.postmanApiKey){
print.lf('Postman API Key was not provided , cannot upload newman run w/o Postman API Key');
return PROCESS_EXIT_FAIL;
/**
* TBA
* @param {*} runSummary
* @param {*} collectionUID
* @param {*} environmentUID
*/
buildRunObject = ( runSummary, collectionUID , environmentUID ) => {}

/**
* @private
* Internal upload call
*
* @param {Object} uploadOptions
* @returns {function} Returns an async function which can be used by async-retry library to have retries
*/
_upload = (uploadOptions) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why does it need to return a function? Couldn't this function itself be used in async/retry?

Copy link
Member

Choose a reason for hiding this comment

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

Hint: .bind()

return async(bail) => await new Promise((resolve, reject) => request.post(uploadOptions , (error, response, body) => {
Copy link
Member

Choose a reason for hiding this comment

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

If you're already returning a Promise, you don't need to await.

Suggested change
return async(bail) => await new Promise((resolve, reject) => request.post(uploadOptions , (error, response, body) => {
return (bail) => new Promise((resolve, reject) => request.post(uploadOptions , (error, response, body) => {

if(error){
if(error.code === 'ECONNREFUSED') return reject(error) // Retry only if the ERROR is ERRCONNECT
return bail(error); // For other errors , dont retry
}

if(200 <= response.statusCode && response.statusCode <= 299){
Copy link
Member

Choose a reason for hiding this comment

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

This seems weird. We should know the discreet status codes that are returned by the API and check on those instead of just checking the range.

return resolve(body);
}

if( 400 <= response.statusCode && response.statusCode<= 499){
Copy link
Member

Choose a reason for hiding this comment

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

Similar as above:

We should know the discreet status codes that are returned by the API and check on those instead of just checking the range.


if(response.statusCode === 404){
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent spacing.

Suggested change
if(response.statusCode === 404){
if (response.statusCode === 404) {

return bail(new Error('Couldn\'t find the postman server route'))
}

return bail(new Error(body.message)); // Avoid retry if there is client side error ( API key error / Workspace ID / permission error)

}

if( 500 <= response.statusCode && response.statusCode <= 599){ // Perform Retry if Server side Error
Copy link
Member

Choose a reason for hiding this comment

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

Similar as above:

We should know the discreet status codes that are returned by the API and check on those instead of just checking the range.

return reject(`Retrying because of server error: ${body.message}`);
}

return reject(); // This should not be activated ( Discuss with Harsh , how to handle 3xx )
})
);
}

print.lf('Uploading newman run to Postman');
/**
* @private
* Internal upload function which handles the retry
*
* @param {*} uploadOptions
* @param {*} retryOptions
* @returns {Promise}
*/
_uploadWithRetry = (upload, retryOptions) => {
return retry( upload,{
Copy link
Member

Choose a reason for hiding this comment

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

Weird spacing.

Suggested change
return retry( upload,{
return retry(upload, {

retries: retryOptions.maxRetries,
factor: retryOptions.retryDelayMultiplier,
randomize: retryOptions.addJitter || false,
maxRetryTime: retryOptions.maxRetryDelay * 1000 , // converting to ms
maxTimeout: retryOptions.totalTimeout * 1000, // converting to ms
onRetry: retryOptions.onRetry || function(){} // e
});
}

const run = buildRunObject(runSummary, uploadConfig.publishWorkspaceSkipResponse);
/**
* @public
* Starts the run upload process
*
* @returns {Boolean} Indicate if run upload was successful
*/
start = async () => {
if(!this.uploadConfig.publishWorkspace){
return false;
}

if(!this.uploadConfig.postmanApiKey){
print.lf('Postman API Key was not provided , cannot upload newman run w/o Postman API Key');
return false;
}

const uploadOptions = {
method: 'POST',
url: UPLOAD_RUN_API_URL,
body: JSON.stringify(run),
headers: {
'Content-Type': 'application/json',
'X-API-Header': uploadConfig.postmanApiKey
print.lf('Uploading newman run to Postman');

const run = this.buildRunObject(runSummary, uploadConfig.publishWorkspaceSkipResponse);

const uploadOptions = {
url: UPLOAD_RUN_API_URL,
body: JSON.stringify(run),
Copy link
Member

Choose a reason for hiding this comment

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

JSON.stringify will throw in some cases. Verify that those cases won't ever occur or catch the error to handle those cases.

Copy link
Author

Choose a reason for hiding this comment

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

Added a try catch block

headers: {
'Content-Type': 'application/json',
'X-API-Header': uploadConfig.postmanApiKey
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct key for key header? Shouldn't it be X-Api-Key? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

}
},
retryOptions = {
maxRetries: uploadConfig.publishRetry,
totalTimeout: uploadConfig.publishUploadTimeout,
retryDelayMultiplier: 2,
maxRetryDelay : 64,
addJitter: true
};

try{
const response = await this._uploadWithRetry(this._upload(uploadOptions), retryOptions)

print.lf(`Uploaded the newman run to postman.
Visit ${response.message} to view the results in postman web`);
return true;

} catch(error) {
Copy link
Member

Choose a reason for hiding this comment

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

Let the caller catch the error and show appropriate message.

Copy link
Author

Choose a reason for hiding this comment

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

Reworked the flow

print.lf(`Unable to upload the results to Postman:
Reason: ${error.message}
You can find solutions to common upload errors here: ${ERROR_GO_TO_LINK}`);
return false;
}
},
retryOptions = {
maxRetries: uploadConfig.publishRetry,
totalTimeout: uploadConfig.publishUploadTimeout,
retryDelayMultiplier: 2,
maxRetryDelay : 64,
addJitter: true
};

try{
const response = await _uploadWithRetry(uploadOptions, retryOptions)

print.lf(`Uploaded the newman run to postman.
Visit ${response.message} to view the results in postman web`);
return PROCESS_EXIT_SUCCESS;

} catch(error) {
print.lf(`Unable to upload the results to Postman:
Reason: ${error.message}
You can find solutions to common upload errors here: ${ERROR_GO_TO_LINK}`);
return PROCESS_EXIT_FAIL;
}
}

Expand Down
10 changes: 10 additions & 0 deletions test/library/run-options.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,4 +239,14 @@ describe('Newman run options', function () {
});
});
});

describe('Newman Run Uploads', function () {
it('should upload Newman run to Postman if API Key and WorksSpaceID are correct', function() {
// @TODO
});

it('should NOT upload Newman run to Postman if API Key and WorkSpaceID are incorrect', function() {
// @TODO
});
});
});
19 changes: 7 additions & 12 deletions test/unit/upload-run.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,12 @@ const uploadRunToPostman = require('../../lib/run/upload-run');
let sandbox;

describe.only('Upload Newman Runs to Postman', function () {
this.beforeEach(function() {
sandbox = require('sinon').createSandbox();
});

this.afterEach(function() {
sandbox.restore();
});

describe('_uploadWithRetry', function () {
beforeEach(function() {
sandbox = require('sinon').createSandbox();
});

afterEach(function () {
sandbox.restore();
});
Expand Down Expand Up @@ -138,28 +135,26 @@ describe.only('Upload Newman Runs to Postman', function () {
const uploadConfig = {
postmanApiKey: 'randomAPIKey'
},
PROCESS_EXIT_SUCCESS = 0,
uploadToPostman = new uploadRunToPostman(uploadConfig),
uploadRetrySpy = sandbox.spy(uploadToPostman._uploadWithRetry);

const exitCode = await uploadToPostman.start();
const uploadStatus = await uploadToPostman.start();

assert.isTrue(uploadRetrySpy.notCalled, 'We cannot upload newman run if dont have publishWorkspace param');
assert.equal(exitCode, PROCESS_EXIT_SUCCESS, 'Newman should exit with a 0 if we dont have a publishWorkspace param');
assert.isFalse(uploadStatus, 'Newman should exit with a 0 if we dont have a publishWorkspace param');
});

it('doesn\'t upload if postman-api-key param is NOT present', async function(){
const uploadConfig = {
publishWorkspace: 'randomWorkspaceID'
},
PROCESS_EXIT_FAIL = 1,
uploadToPostman = new uploadRunToPostman(uploadConfig),
uploadRetrySpy = sandbox.spy(uploadToPostman._uploadWithRetry);

const exitCode = await uploadToPostman.start();

assert.isTrue(uploadRetrySpy.notCalled, 'We cannot upload newman run if dont have publishWorkspace param');
assert.equal(exitCode, PROCESS_EXIT_FAIL, 'Newman should exit with a 1 if we dont have a postmanApiKey param');
assert.isFalse(exitCode, 'Newman should exit with a 1 if we dont have a postmanApiKey param');
});

});
Expand Down