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 10 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
12 changes: 8 additions & 4 deletions bin/newman.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,12 @@ 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 <workspace-id>', 'Publishes to given workspace')
Copy link
Member

Choose a reason for hiding this comment

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

Could it simply be --publish? From the perspective of user, I feel if we have a option like --publish-workspace, we would have another option --publish which enables publishing. But this option itself is doing that, no?

Also, publish-workspace feels a little like we're publishing a workspace. 😅

Thoughts?

cc @codenirvana

Choose a reason for hiding this comment

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

I agree, let's keep it --publish

Choose a reason for hiding this comment

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

and --publish-skip-response

.option('--publish-workspace-skip-response', 'Skip responses (headers, body, etc) while uploading newman run to Postman')
Copy link
Member

Choose a reason for hiding this comment

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

What does "workspace" mean in this option? Could it simply be --publish-skip-response?

Copy link
Author

Choose a reason for hiding this comment

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

Changed all the CLI parameter names

.option('--publish-retry', 'Number of times newman can try to publish before safely erroring out.')
Copy link
Member

Choose a reason for hiding this comment

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

--publish-retries would make more sense, I think. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense

.option('--publish-upload-timeout', 'Timeout for uploading newman runs to postman', util.cast.integer, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Could simply be --publish-timeout, I think.

.action((collection, command) => {
let options = util.commanderToObject(command),

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

Expand All @@ -74,14 +77,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);
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 incorrect. If the runError is true and resultUploaded is also true, it would not exit the process with status 1, which it should given that the run has errored.

Copy link
Member

Choose a reason for hiding this comment

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

This should be an ||.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed this

});
});

Expand Down Expand Up @@ -139,7 +143,7 @@ function run (argv, callback) {

// This hack has been added from https://github.com/nodejs/node/issues/6456#issue-151760275
// @todo: remove when https://github.com/nodejs/node/issues/6456 has been fixed
(Number(process.version[1]) >= 6) && [process.stdout, process.stderr].forEach((s) => {
Number(process.version[1]) >= 6 && [process.stdout, process.stderr].forEach((s) => {
UmedJadhav marked this conversation as resolved.
Show resolved Hide resolved
s && s.isTTY && s._handle && s._handle.setBlocking && s._handle.setBlocking(true);
});

Expand Down
18 changes: 15 additions & 3 deletions lib/run/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var _ = require('lodash'),
getOptions = require('./options'),
exportFile = require('./export-file'),
util = require('../util'),
uploadRunToPostman = require('./upload-run'),

/**
* This object describes the various events raised by Newman, and what each event argument contains.
Expand Down Expand Up @@ -95,7 +96,12 @@ module.exports = function (options, callback) {
var emitter = new EventEmitter(), // @todo: create a new inherited constructor
runner = new runtime.Runner(),
stopOnFailure,
entrypoint;
entrypoint,
uploadConfig = _.pick(options, ['postmanApiKey', 'publishWorkspace', 'publishWorkspaceSkipResponse',
'publishUploadTimeout', 'publishRetry']);

uploadConfig.collectionUID = util.extractResourceUID(options.collection);
uploadConfig.environmentUID = util.extractResourceUID(options.environment);
Copy link
Member

Choose a reason for hiding this comment

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

The concept of uid does not exist outside of postman. It can simply be id.


// get the configuration from various sources
getOptions(options, function (err, options) {
Expand Down Expand Up @@ -302,6 +308,9 @@ 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

// TODO: Add analytics call here

if (err) {
emitter.emit('done', err, emitter.summary);
callback(err, emitter.summary);
Expand Down Expand Up @@ -340,10 +349,13 @@ module.exports = function (options, callback) {
});
});

asyncEach(emitter.exports, exportFile, function (err) {
asyncEach(emitter.exports, exportFile, async function (err) {
Copy link
Member

Choose a reason for hiding this comment

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

Using callbacks and async/await in the same function seems like an anti-pattern. I would recommend if we stick to callbacks to maintain consistency. Thoughts?

// we now trigger actual done event which we had overridden
emitter.emit('done', err, emitter.summary);
callback(err, emitter.summary);

var resultUploaded = await new uploadRunToPostman(uploadConfig, emitter.summary).start();
Copy link
Member

Choose a reason for hiding this comment

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

This could be just a method call instead if we only ever need one method from the instance of the class. Why create a class for this at all?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var resultUploaded = await new uploadRunToPostman(uploadConfig, emitter.summary).start();
new uploadRunToPostman(uploadConfig, emitter.summary).start().then((resultUpdated) =>


callback(err, emitter.summary, { resultUploaded });
});
}
});
Expand Down
133 changes: 133 additions & 0 deletions lib/run/upload-run.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
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'),
ERROR_GO_TO_LINK = '<TBA>',
UPLOAD_RUN_API_URL = '<TBA>'

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.


constructor(uploadConfig, runSummary){
this.uploadConfig = uploadConfig,
this.runSummary = runSummary;
}

/**
* TBA after discussing with Harsh
* @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 )
})
);
}

/**
* @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
});
}

/**
* @public
* Starts the run upload process
*
* @returns {Boolean} Indicate if run upload was successful
*/
start = async () => {
if(!this.uploadConfig.publishWorkspace){
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this check be made before calling the start method itself? 🤔

Also, since this is an async function. It would be better to reject the returned promise by throw-ing here (and everywhere you want to return false) with appropriate error message.

Copy link
Author

Choose a reason for hiding this comment

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

Moved the check to run/index.js

}

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

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;
}
}
}

module.exports = uploadRunToPostman;
31 changes: 31 additions & 0 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,37 @@ 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];
}
};

Expand Down
13 changes: 13 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"license": "Apache-2.0",
"dependencies": {
"async": "3.2.1",
"async-retry": "^1.3.3",
Copy link
Member

Choose a reason for hiding this comment

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

The retry is present in the async library itself. You don't need to add another dependency. Ref: https://caolan.github.io/async/v3/docs.html#retry

"chardet": "1.3.0",
"cli-progress": "3.9.0",
"cli-table3": "0.6.0",
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
});
});
});
Loading