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

Feature: Added a new default reporter to publish newman run to postman #2978

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

Harsh-Bansal
Copy link

This default reporter can be used to publish newman run to postman.
This reporter doesn't have any associated options.

This reporter can be used to publish newman run to postman.
@Harsh-Bansal Harsh-Bansal self-assigned this May 26, 2022
@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #2978 (18e8fe3) into develop (a5018ec) will increase coverage by 0.28%.
The diff coverage is 93.66%.

@@             Coverage Diff             @@
##           develop    #2978      +/-   ##
===========================================
+ Coverage    90.96%   91.25%   +0.28%     
===========================================
  Files           21       25       +4     
  Lines         1151     1292     +141     
  Branches       349      380      +31     
===========================================
+ Hits          1047     1179     +132     
- Misses         104      113       +9     
Flag Coverage Δ
cli 76.16% <27.46%> (-6.03%) ⬇️
integration 39.31% <26.05%> (-1.69%) ⬇️
library 55.26% <28.87%> (-3.30%) ⬇️
unit 78.40% <93.66%> (+3.34%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/run/index.js 93.75% <ø> (ø)
lib/reporters/postman/helpers/upload-run.js 83.33% <83.33%> (ø)
lib/reporters/postman/index.js 93.54% <93.54%> (ø)
lib/reporters/postman/helpers/run-utils.js 95.00% <95.00%> (ø)
lib/reporters/postman/helpers/constants.js 100.00% <100.00%> (ø)
lib/run/options.js 95.45% <100.00%> (+0.03%) ⬆️
lib/util.js 96.29% <100.00%> (+1.05%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Harsh-Bansal
Copy link
Author

@anshuljain03 will be pushing another commit for tests.
Core logic of the reporter is done, might update a few error msgs.

1. Moved upload-run to helpers folder
2. Code cleanup
3. Better error messages
@@ -42,7 +42,8 @@ var _ = require('lodash'),
json: require('../reporters/json'),
junit: require('../reporters/junit'),
progress: require('../reporters/progress'),
emojitrain: require('../reporters/emojitrain')
emojitrain: require('../reporters/emojitrain'),
postman: require('../reporters/postman')
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure the usage should be -r postman? What happened to --publish?

If yes, why it's named postman? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Using the -r postman follows the reporter convention which seems much better. Deprecating --publish would have been difficult (if needed).
Also, later when we add other configurable params (like timeout, retries), this approach would prevent bloating of newman cli options. They being specific to the reporter wont really belong as params of newman which doesnt use these (better separation of concerns on the interface)

Why postman: We went with postman as the data is being pushed to postman servers. (similar to other external reporters mentioned here (https://postmanlabs.atlassian.net/wiki/spaces/RUN/pages/3890544729))

We also considered cloudpublish/postmanpublish/postmancloud but postman seemed better.
Please let me know if you have any suggestions on this.


// We get the collection id in the collectionRunOptions. But that collection id has the user id stripped off
// Hence, we try to parse the CLI args to extract the whole collection id from it.
const processArgs = process.argv,
Copy link
Member

Choose a reason for hiding this comment

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

How will this work when Newman is used as a library?

Copy link
Author

Choose a reason for hiding this comment

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

This will be available only via CLI.
The issue is we need the collection UID to publish runs (wrt a collection) and we can get it from CLI args but when used as a library we don't have a way to get it. (The reporter gets passed the collection object fetched via postman api which doesnt have the UID)

Will figure out a way to use it as part of library also in the second iteration.
(Our current target use-case is CLI invocations as part of CI/CD)

Copy link
Author

Choose a reason for hiding this comment

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

@codenirvana have made the changes to make the reporter compatible with library also.
cc @anshuljain03 @shubhbhargav

collectionRunOptions.postmanApiKey || util.getAPIKeyFromCLIArguments(processArgs);

if (!collection) {
print.lf('Publishing run details to postman cloud is currently supported only for collections specified ' +
Copy link
Member

Choose a reason for hiding this comment

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

Are we following the Newman logging pattern here?

/**
* The base URL for postman API
*/
POSTMAN_API_BASE_URL: 'https://api.getpostman.com',
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 using the older getpostman.com domain name?

Copy link
Author

Choose a reason for hiding this comment

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

Switched to postman.com, thanks
Have also modified related regexs to support both getpostman.com and postman.com.

/**
* Used as a fall back error message for the upload API call
*/
RESPONSE_FALLBACK_ERROR_MESSAGE: 'Something went wrong while uploading newman run data 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.

No, "Something went" error messages, please.

/**
* Regex pattern to extract the api key from the postman api collection url
*/
API_KEY_FROM_URL_EXTRACTION_PATTERN:
Copy link
Member

Choose a reason for hiding this comment

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

Avoid this!

Copy link
Author

Choose a reason for hiding this comment

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

@codenirvana we are using this only as a fallback in case apikey is not present as environment variable POSTMAN_API_KEY or passed as cli arg --postman-api-key

Lemmi know if that's fine.

time: response.responseTime,
size: response.responseSize,
headers: headers,
body: response.stream ? new TextDecoder('utf-8').decode(new Uint8Array(response.stream)) : null
Copy link
Member

Choose a reason for hiding this comment

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

response.text()

1. Changed fallback error message
2. Changed POSTMAN_API_BASE_URL to latest domain
3. Modified regex patterns to comply with both old and new domain
4. Modified _buildResponseObject to use response.text()
…t uids and apikey from postman reporter.

This info will now be stored in options which is sent to the reporter.
Moved utility functions to extract the above from postman reporter to newman util.
Added test cases for the utility functions.
Cleanup for existing tests based on above.
@@ -53,7 +53,19 @@ var fs = require('fs'),

// Matches valid Postman UID, case insensitive.
// Same used for validation on the Postman API side.
UID_REGEX = /^[0-9A-Z]+-[0-9A-F]{8}-[0-9A-F]{4}-[0-9A-F]{4}-[0-9A-F]{4}-[0-9A-F]{12}$/i;
Copy link
Member

Choose a reason for hiding this comment

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

@codenirvana ... I think we should start moving away from having internal reporters using non public API. For example, the CLI reporter uses util too. That is what's prompting this. Instead, we should try opening these needed utils as reporter util exposed via newman itself.

Thoughts?

@@ -380,6 +380,13 @@ module.exports = function (options, callback) {
// allow insecure file read by default
options.insecureFileRead = Boolean(_.get(options, 'insecureFileRead', true));

// store collection, environment uid and postman api key in options if specified using postman public api link
options.cachedArgs = {
Copy link
Member

Choose a reason for hiding this comment

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

Premature execution. Alternate source available for this data from SDK instances inside. cachedArgs elevated/leaked in reporter facing public API

UID_REGEX = /^[0-9A-Z]+-[0-9A-F]{8}-[0-9A-F]{4}-[0-9A-F]{4}-[0-9A-F]{4}-[0-9A-F]{12}$/i,

// Regex pattern to extract the collection id from the postman api collection url
COLLECTION_UID_FROM_URL_EXTRACTION_PATTERN =
Copy link
Member

Choose a reason for hiding this comment

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

ReDos vulnerable RegEx

/https?:\/\/api\.(?:get)?postman.*\.com\/(?:collections)\/([A-Za-z0-9-]+)/,

// Regex pattern to extract the environment id from the postman api environment url
ENVIRONMENT_UID_FROM_URL_EXTRACTION_PATTERN =
Copy link
Member

Choose a reason for hiding this comment

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

ReDOS here probably too


// Regex pattern to extract the api key from the postman api collection url
API_KEY_FROM_URL_EXTRACTION_PATTERN =
/https?:\/\/api.(?:get)?postman.com\/[a-z]+s\/[a-z0-9-]+\?apikey=([a-z0-9A-Z-]+)/;
Copy link
Member

Choose a reason for hiding this comment

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

Seems not REDOS vulnerable 👍

@@ -284,6 +296,76 @@ util = {
*/
isFloat: function (value) {
return (value - parseFloat(value) + 1) >= 0;
},
Copy link
Member

Choose a reason for hiding this comment

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

Not relevant to global util.

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2024

Codecov Report

Attention: Patch coverage is 93.66197% with 9 lines in your changes missing coverage. Please review.

Project coverage is 91.25%. Comparing base (a5018ec) to head (18e8fe3).
Report is 87 commits behind head on develop.

Files with missing lines Patch % Lines
lib/reporters/postman/helpers/upload-run.js 83.33% 4 Missing ⚠️
lib/reporters/postman/helpers/run-utils.js 95.00% 3 Missing ⚠️
lib/reporters/postman/index.js 93.54% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2978      +/-   ##
===========================================
+ Coverage    90.96%   91.25%   +0.28%     
===========================================
  Files           21       25       +4     
  Lines         1151     1292     +141     
  Branches       349      380      +31     
===========================================
+ Hits          1047     1179     +132     
- Misses         104      113       +9     
Flag Coverage Δ
cli 76.16% <27.46%> (-6.03%) ⬇️
integration 39.31% <26.05%> (-1.69%) ⬇️
library 55.26% <28.87%> (-3.30%) ⬇️
unit 78.40% <93.66%> (+3.34%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants