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 13 commits
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
13 changes: 10 additions & 3 deletions bin/newman.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,15 @@ program
.option('--cookie-jar <path>', 'Specify the path to a custom cookie jar (serialized tough-cookie JSON) ')
.option('--export-cookie-jar <path>', 'Exports the cookie jar to a file after completing the run')
.option('--verbose', 'Show detailed information of collection run and each request sent')
.option('-p, --publish <workspace-id>', 'Publishes to given workspace')
.option('--publish-skip-response',
'Skip responses (headers, body, etc) while uploading newman run to Postman')
.option('--publish-retries', 'Number of times newman can try to publish before safely erroring out.',
util.cast.integer, 3)
.option('--publish-timeout', 'Timeout in seconds for uploading newman runs to postman',
util.cast.integer, 60)
.action((collection, command) => {
let options = util.commanderToObject(command),

// parse custom reporter options
reporterOptions = util.parseNestedOptions(program._originalArgs, '--reporter-', options.reporters);

Expand All @@ -74,14 +80,15 @@ program
acc[key] = _.assignIn(value, reporterOptions._generic); // overrides reporter options with _generic
}, {});

newman.run(options, function (err, summary) {
newman.run(options, function (err, summary, newmanStatus) {
const runError = err || summary.run.error || summary.run.failures.length;

if (err) {
console.error(`error: ${err.message || err}\n`);
err.friendly && console.error(` ${err.friendly}\n`);
}
runError && !_.get(options, 'suppressExitCode') && process.exit(1);

(runError || !newmanStatus.resultUploaded) && !_.get(options, 'suppressExitCode') && process.exit(1);
});
});

Expand Down
25 changes: 23 additions & 2 deletions lib/run/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ var _ = require('lodash'),
getOptions = require('./options'),
exportFile = require('./export-file'),
util = require('../util'),
print = require('../print'),
{ uploadRunToPostman, POSTMAN_UPLOAD_ERROR_LINK } = require('./upload-run'),

/**
* This object describes the various events raised by Newman, and what each event argument contains.
Expand Down Expand Up @@ -302,7 +304,10 @@ module.exports = function (options, callback) {
// in case runtime faced an error during run, we do not process any other event and emit `done`.
// we do it this way since, an error in `done` callback would have anyway skipped any intermediate
// events or callbacks
if (err) {

// TODO: Add analytics call here

if (err) { // TODO -> understand when this is raised
Copy link
Member

@coditva coditva Oct 6, 2021

Choose a reason for hiding this comment

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

@UmedJadhav This error is raised from the postman-runtime library if the collection run did not finish successfully.

emitter.emit('done', err, emitter.summary);
callback(err, emitter.summary);

Expand Down Expand Up @@ -343,7 +348,23 @@ module.exports = function (options, callback) {
asyncEach(emitter.exports, exportFile, function (err) {
// we now trigger actual done event which we had overridden
emitter.emit('done', err, emitter.summary);
callback(err, emitter.summary);
let publishSummary;

return uploadRunToPostman(emitter.summary, options)
.then((response) => {
publishSummary = { uploadSuccessful: true, resultUrl: response.message,
runPayloadSize: response.runPayloadSize };
print.lf(`Uploaded the newman run to postman.
Visit ${publishSummary.resultUrl} to view the results in postman web`);
})
.catch((error) => {
publishSummary = { uploadSuccessful: false, error: error.message };
print.lf(`Unable to upload the results to Postman:
Reason: ${error.message}
You can find solutions to common upload errors here: ${POSTMAN_UPLOAD_ERROR_LINK}`);
}).finally(() => {
callback(err, emitter.summary, publishSummary);
});
});
}
});
Expand Down
237 changes: 237 additions & 0 deletions lib/run/upload-run.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
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'),

POSTMAN_UPLOAD_ERROR_LINK = '<TBA>',
UPLOAD_RUN_API_URL = '<TBA>',
NEWMAN_STRING = 'newman'

/**
* @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.

If this is going to return a function, it should be called something like _getUploadRetryable.
Or, don't return a function! Just bind the params where you're using it.

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

Choose a reason for hiding this comment

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

Indentation looks weird here.

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

// Handle exact status codes
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 seem to be handling the exact status codes. 😅

Copy link
Member

Choose a reason for hiding this comment

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

Should use a switch-case in this case (pun intended.)


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

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

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
return reject(`Retrying server error: ${body.message}`); // Change this after discussion with Harsh
}

return reject(`Recieved an unexpected Response status code while uploading Newman run`); // This should not be activated ( Discuss with Harsh , how to handle 3xx )
})
);
}

/**
* @private
* Internal upload function which handles the retry
*
* @param {*} uploadOptions
* @param {*} retryOptions
*
* @returns {Promise}
*/
_uploadWithRetry = (upload, retryOptions) => {
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
});
}

/**
* @private
*/
_buildRequestObject = (request)=> {
return {
url: request.url, // buildUrl(request.url), // @TODO : Ask utkarsh if we have a function that combines a url object to
method: request.method,
path: '/ Collections / Create Collections / Create a collection into personal workspace' // TODO - Find out where is this used
}
}

/**
* @private
* @param {Object} response
* @param {Boolean} skipResponse
*
* @returns {Object}
*/
_buildResponseObject = (response, skipResponse) => {
if(skipResponse) return

return {
code: response.code,
name: response.status,
time: response.responseTime,
size: response.responseSize,
}
}

/**
* @private
* @param {Array} assertions
*
* @returns {Array}
*/
_buildTestObject = (assertions) => {
const tests = []

assertions && assertions.forEach(assert => {
tests.push({
name: assert.assertion,
error: assert.error || {}, // @TODO - Understand This is the entire object with params - name , index, test, message , stack
status: assert.error ? 'fail' : 'pass',
})
});

return tests
}

/**
* @private
* @param {Array} executions
* @param {Number} iterationCount
* @param {Boolean} skipResponse
*
* @returns {Array}
*/
_executionToIterationConverter = (executions, iterationCount, skipResponse) => {
const iterations = []
executions = util.partition(executions, iterationCount);

executions.forEach( iter => {
const iteration = []

iter.forEach(req => {
iteration.push({
id: req.item.id,
name: req.item.name,
ref: 'uuid4', // @TODO : Figure out how is this used ??
request: _buildRequestObject(req.request),
response: req.response ? _buildResponseObject(req.response, skipResponse): null,
requestError: req.requestError || '',// @TODO - Check for this with Harsh
tests: _buildTestObject(req.assertions), // What's the value if tests are not present for a request
});
});

iterations.push(iteration);

});
return iterations
}

/**
* @private
* @param {Object} runOptions
* @param {Object} runOptions
*
* @returns {String}
*/
_buildPostmanUploadPayload = (runOptions, runSummary) => {
const run = {
name: runOptions.collection.name || '',
status:runSummary.run.error ? 'failed': 'finished', // THis can be finished or failed - Check with Harsh about all the possible values
source: NEWMAN_STRING,
failedTestCount: runSummary.run.stats.assertions.failed || 0,
totalTestCount: runSummary.run.stats.assertions.total || 0,
collection: runOptions.collectionUID || '',
environment: runOptions.environmentUID || '',
iterations: _executionToIterationConverter(runSummary.run.executions, runOptions.iterationCount , runOptions.publishSkipResponse),
delay: runOptions.delayRequest,
persist: false,
saveResponse: !runOptions.publishSkipResponse,
dataFile: runOptions.collectionUID && runOptions.environmentUID ? runOptions.iterationData : null,
workspace: runOptions.publish,
currentIteration: runOptions.iterationCount,// Tells how many iterations where there ,
folder: '' // @TODO - What is this ?
}
return JSON.stringify(run);
}

/**
* @public
* Starts the run upload process
*
* @param {*} runSummary
* @param {*} runOptions
*
* @returns { Promise }
*/
uploadRunToPostman = (runSummary, runOptions) => {

let runPayload;

runOptions.collectionUID = util.extractResourceUID(runOptions.collection);
runOptions.environmentUID = util.extractResourceUID(runOptions.environment);

try{
runPayload = _buildPostmanUploadPayload(runOptions, runSummary)
}catch(error){
throw new Error(`Unable to serialize the run - ${error}`);
}

print.lf('Uploading newman run to Postman');

const uploadOptions = {
url: UPLOAD_RUN_API_URL,
body: runPayload,
headers: {
'Content-Type': 'application/json',
'X-Api-Key': runOptions.postmanApiKey
}
},
retryOptions = {
maxRetries: runOptions.publishRetries,
totalTimeout: runOptions.publishTimeout,
retryDelayMultiplier: 2,
maxRetryDelay : 64,
addJitter: true
};

return _uploadWithRetry(_upload(uploadOptions), retryOptions);
}

module.exports = {
uploadRunToPostman,
POSTMAN_UPLOAD_ERROR_LINK,

// Exporting following functions for testing ONLY.
_executionToIterationConverter,
_buildPostmanUploadPayload,
_buildRequestObject,
_buildResponseObject,
_buildTestObject,
_uploadWithRetry,
_upload,

};
48 changes: 48 additions & 0 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,54 @@ util = {
*/
isFloat: function (value) {
return (value - parseFloat(value) + 1) >= 0;
},

/**
* Helper to get resource ID from URL
*
* If resourceUrl = https://api.getpostman.com/collections/<collection-UID>?apikey=<api_key>
* This method extracts <collection-UID>
*
* @param {String} resource - url for collection\environment
* @param resourceURL
* @returns {String}
*/
extractResourceUID: function (resourceURL) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call it id instead of uid

if (_.isObject(resourceURL) || !resourceURL || resourceURL === '') {
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 like a weird check. If we want the resourceURL to be a string, we should check it as such. Also, the case of it being empty string would not need to be handled explicitly at all since the regex would take care of it.

Suggested change
if (_.isObject(resourceURL) || !resourceURL || resourceURL === '') {
if (typeof resourceURL !== 'string') {

return '';
}

const uidExtractionPattern = /https:\/\/api\.getpostman\.com\/[collections|environments]+\/(\w+)\?apikey=\w+/g;
Copy link
Member

Choose a reason for hiding this comment

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

This regex seems to be incorrect. It works for cases like:

  • https://api.getpostman.com/c/abc?apikey=abc
  • https://api.getpostman.com/e/abc?apikey=abc
  • https://api.getpostman.com/collectionscollections/abc?apikey=abc

And it doesn't work for cases:

  • https://api.getpostman.com/collections/abc
  • https://api.getpostman.com/collections/abc?apikey=
  • https://api.getpostman.com/collections/abc?apikey=
  • https://api.getpostman.com/collections/abc-abc?apikey=abc

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need to check for ?apikey= part? 🤔


let matches,
results = [];

while ((matches = uidExtractionPattern.exec(resourceURL)) !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we looping over all the matches? It should be a single match, no? You would have the match present in the first call to exec itself.

if (matches.index === uidExtractionPattern.lastIndex) {
uidExtractionPattern.lastIndex++;
}

matches.forEach((match) => { return results.push(match); });
}

return results.length === 0 ? '' : results[1];
},

partition: function (arr, n) {
const validPartitionArg = Number.isSafeInteger(n) && n > 0,
partitions = [];

if (!validPartitionArg) {
throw new Error('n must be positive integer');
}

for (let i = 0; i < arr.length; i += n) {
const partition = arr.slice(i, i + n);

partitions.push(partition);
}

return partitions;
}
};

Expand Down
Loading