-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: develop
Are you sure you want to change the base?
Conversation
- This utility will be used to upload the newman runs to postman - We will be using exponential retry so as to not bombard the postman servers
Codecov Report
@@ Coverage Diff @@
## develop #2849 +/- ##
============================================
- Coverage 90.96% 13.00% -77.97%
============================================
Files 21 22 +1
Lines 1151 1269 +118
Branches 349 383 +34
============================================
- Hits 1047 165 -882
- Misses 104 1104 +1000
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
bin/newman.js
Outdated
@@ -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') | |||
.option('--publish-workspace-skip-response', 'Skip responses (headers, body, etc) while uploading newman run to Postman') | |||
.option('--publish-retry', 'Number of times newman can try to publish before safely erroring out.') |
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.
--publish-retries
would make more sense, I think. Thoughts?
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.
Makes sense
bin/newman.js
Outdated
@@ -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') | |||
.option('--publish-workspace-skip-response', 'Skip responses (headers, body, etc) while uploading newman run to Postman') |
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 does "workspace" mean in this option? Could it simply be --publish-skip-response
?
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.
Changed all the CLI parameter names
bin/newman.js
Outdated
@@ -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') |
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.
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
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 agree, let's keep it --publish
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.
and --publish-skip-response
bin/newman.js
Outdated
.option('-p, --publish-workspace <workspace-id>', 'Publishes to given workspace') | ||
.option('--publish-workspace-skip-response', 'Skip responses (headers, body, etc) while uploading newman run to Postman') | ||
.option('--publish-retry', 'Number of times newman can try to publish before safely erroring out.') | ||
.option('--publish-upload-timeout', 'Timeout for uploading newman runs to postman', util.cast.integer, 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.
Could simply be --publish-timeout
, I think.
bin/newman.js
Outdated
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); |
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 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.
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 should be an ||
.
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.
runError && !newmanStatus.resultUploaded && !_.get(options, 'suppressExitCode') && process.exit(1); | |
(runError || !newmanStatus.resultUploaded) && !_.get(options, 'suppressExitCode') && process.exit(1); |
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.
Fixed this
lib/run/upload-run.js
Outdated
* @returns {Promise} | ||
*/ | ||
_uploadWithRetry = (upload, retryOptions) => { | ||
return retry( upload,{ |
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.
Weird spacing.
return retry( upload,{ | |
return retry(upload, { |
lib/run/upload-run.js
Outdated
*/ | ||
start = async () => { | ||
if(!this.uploadConfig.publishWorkspace){ | ||
return true; |
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.
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.
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.
Moved the check to run/index.js
lib/run/upload-run.js
Outdated
Visit ${response.message} to view the results in postman web`); | ||
return true; | ||
|
||
} catch(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.
Let the caller catch the error and show appropriate message.
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.
Reworked the flow
lib/run/upload-run.js
Outdated
|
||
const uploadOptions = { | ||
url: UPLOAD_RUN_API_URL, | ||
body: JSON.stringify(run), |
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.stringify
will throw in some cases. Verify that those cases won't ever occur or catch the error to handle those cases.
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 a try catch block
lib/run/upload-run.js
Outdated
body: JSON.stringify(run), | ||
headers: { | ||
'Content-Type': 'application/json', | ||
'X-API-Header': uploadConfig.postmanApiKey |
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.
Is this the correct key for key header? Shouldn't it be X-Api-Key
? 🤔
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.
Fixed
|
||
// TODO: Add analytics call here | ||
|
||
if (err) { // TODO -> understand when this is raised |
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.
@UmedJadhav This error is raised from the postman-runtime
library if the collection run did not finish successfully.
*/ | ||
_upload = (uploadOptions) => { | ||
return async (bail) => request.post(uploadOptions , (error, response, body) => { | ||
if(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.
Indentation looks weird here.
_upload = (uploadOptions) => { | ||
return async (bail) => request.post(uploadOptions , (error, response, body) => { | ||
if(error){ | ||
if(error.code === 'ECONNREFUSED') { // Retry only if the ERROR is ERRCONNECT |
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: 'ECONNREFUSED'
should be a const
.
lib/run/upload-run.js
Outdated
return async (bail) => request.post(uploadOptions , (error, response, body) => { | ||
if(error){ | ||
if(error.code === 'ECONNREFUSED') { // Retry only if the ERROR is ERRCONNECT | ||
throw new Error(error.message); |
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 the error message is same, why not just throw the error itself?
throw new Error(error.message); | |
throw error; |
return bail(error); // For other errors , dont retry | ||
} | ||
|
||
// Handle exact status codes |
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.
We don't seem to be handling the exact status codes. 😅
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.
Should use a switch-case
in this case (pun intended.)
* @param {Object} uploadOptions | ||
* @returns {function} Returns an async function which can be used by async-retry library to have retries | ||
*/ | ||
_upload = (uploadOptions) => { |
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 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.
Adding the ability to upload newman run results to Postman .
Things done
-
--publish-workspace <workspace-id>
-
--publish-workspace-skip-response
-
--publish-retry
-
--publish-upload-timeout
To-Do List
Refer to the following image for data flow