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

Added flag to set verbose output size limit for newman run #2883

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

Conversation

Sid200026
Copy link

@Sid200026 Sid200026 commented Dec 8, 2021

Flag for Verbose Output Size Limit

Previously, the output limit when the verbose flag was set was 2KB or 2048 bytes which were hardcoded in the codebase. The main argument that can be made for hardcoding the limit to 2KB is large data streams can crash the terminal. However, this limit is unfair in cases when the output size is just beyond the 2KB size limit. Moreover in cases when you want to pipe the output of newman run to a file, setting a size limit doesn't make sense and moreover, the terminal won't crash.

This PR adds two new flags under --reporter-cli-*

  1. --reporter-cli-verbose-output-limit : Specify the truncation size of body in verbose CLI output for requests and responses
  2. --reporter-cli-no-verbose-output-limit : This removes any truncation size limit of body in verbose CLI output for requests and responses

Syntax

newman run <collection> --verbose --reporter-cli-verbose-output-limit  <size in any unit>
newman run <collection> --verbose --reporter-cli-no-verbose-output-limit

Changes

  • Modified cli core logic
  • Added tests
  • Updated Documentation

Closes #2845

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #2883 (a66db04) into develop (90f4449) will increase coverage by 0.59%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2883      +/-   ##
===========================================
+ Coverage    90.96%   91.55%   +0.59%     
===========================================
  Files           21       21              
  Lines         1151     1161      +10     
  Branches       349      352       +3     
===========================================
+ Hits          1047     1063      +16     
+ Misses         104       98       -6     
Flag Coverage Δ
cli 82.77% <100.00%> (+0.67%) ⬆️
integration 40.74% <7.14%> (-0.27%) ⬇️
library 58.13% <7.14%> (-0.42%) ⬇️
unit 74.50% <7.14%> (-0.57%) ⬇️

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

Impacted Files Coverage Δ
bin/util.js 100.00% <ø> (ø)
lib/reporters/cli/index.js 87.43% <100.00%> (+3.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90f4449...a66db04. Read the comment docs.

Copy link
Member

@shamasis shamasis left a comment

Choose a reason for hiding this comment

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

Also, this codecov seems to be failing. We did cross the threshold in the PR, no clue why it's still failing.

].join(` ${colors.gray(symbols.star)} `);
].join(` ${colors.gray(symbols.star)} `),

bodyClipSize = options.outputSize * KB_TO_BYTE_MULTIPLIER;
Copy link
Member

Choose a reason for hiding this comment

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

Handle fallback for invalid bodySize in-line here

README.md Outdated
@@ -174,6 +174,9 @@ For more details on [Reporters](#reporters) and writing your own [External Repor
- `--timeout-script <ms>`<br />
Specify the time (in milliseconds) to wait for scripts to complete execution.

- `--output-size <KB>`<br />
Copy link
Member

Choose a reason for hiding this comment

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

As we all know, naming is the hardest part of the equation. This name has some way to go to resonate with what it does - "specify the truncation point of a body in verbose CLI output for requests and responses."

How about bracketing it under "reporter-cli-*" params?

Copy link
Author

Choose a reason for hiding this comment

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

Having gone through the reporter-cli params, I think bracketing it under reporter-cli-* would be more appropriate
How about replacing the current output-size flag with two reporter-cli flags

  • reporter-cli-verbose-output-limit <Size in any memory unit>
  • reporter-cli-no-verbose-output limit : Sets the limit to Infinite

It's rare to see Command Line tools allow users to pass arguments as infinite. Plus spelling errors might creep in.Mostly, they achieve by simply setting some flag.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @shamasis, any thoughts regarding this?

README.md Outdated
@@ -288,6 +291,7 @@ return of the `newman.run` function is a run instance, which emits run events th
| options.timeout | Specify the time (in milliseconds) to wait for the entire collection run to complete execution.<br /><br />_Optional_<br />Type: `number`, Default value: `Infinity` |
| options.timeoutRequest | Specify the time (in milliseconds) to wait for requests to return a response.<br /><br />_Optional_<br />Type: `number`, Default value: `Infinity` |
| options.timeoutScript | Specify the time (in milliseconds) to wait for scripts to return a response.<br /><br />_Optional_<br />Type: `number`, Default value: `Infinity` |
| options.outputSize | Specify the output size limit ( in KB ) when verbose flag is set.<br /><br />For example, `outputSize = 2` sets the verbose output limit to 2KB or 2048 bytes.<br /><br />To remove any verbose output limit, pass `infinite` to `--output-size`. This would set `outputSize` to `Number. POSITIVE_INFINITY`<br /><br />_Optional_<br />Type: `number`, Default value: `2` |
Copy link
Member

@shamasis shamasis Dec 8, 2021

Choose a reason for hiding this comment

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

Is KB the right unit? Any other option around size that we can draw pattern from?

Any way we can take more dynamic input by parsing units? I'm sure there will be some popular library that can take "10kb", "1Mb" as inputs and spit out absolute bytes. 😛

Copy link
Author

Choose a reason for hiding this comment

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

Sure, will implement this. In case of invalid inputs, should we proceed with default 2KB or throw an error?

bin/newman.js Outdated
@@ -50,6 +50,7 @@ program
.option('--timeout [n]', 'Specify a timeout for collection run (milliseconds)', util.cast.integer, 0)
.option('--timeout-request [n]', 'Specify a timeout for requests (milliseconds)', util.cast.integer, 0)
.option('--timeout-script [n]', 'Specify a timeout for scripts (milliseconds)', util.cast.integer, 0)
.option('--output-size [n]', 'Specify the output size limit for CLI ( in KB )', util.cast.integerOrInfinity, 2)
Copy link
Member

Choose a reason for hiding this comment

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

English here is ambiguous "output size of what? Entire CLI, body always, body in verbose"?!

Copy link
Author

Choose a reason for hiding this comment

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

Technically, this PR limits the verbose body output only. I'll make the necessary changes to remove this ambiguity.

bin/util.js Outdated
* @param {String} arg - The stringified number argument or infinite.
* @returns {Number} - The supplied argument, casted to an integer or positive infinity.
*/
integerOrInfinity: (arg) => {
Copy link
Member

Choose a reason for hiding this comment

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

How does Newman handle other properties that can have infinity? Any pattern to draw from there?

@@ -33,4 +37,37 @@ describe('newman run --verbose', function () {
done();
});
});

it('should limit to 2KB with --verbose option only without setting output-size', function (done) {
Copy link
Member

Choose a reason for hiding this comment

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

Very neat and creative test. Like it ❤️😊.

Another improvement could be if the body was autogenerated series of number marking increasing size. That way, we can both assert points and accuracy.

@Sid200026
Copy link
Author

@shamasis can you provide me with some additional feedback so that I can continue working on this?

@Sid200026
Copy link
Author

Hey @shamasis, @codenirvana @coditva, I have updated this PR with the necessary changes.

Currently there are 2 flags,

  1. newman run <collection> --verbose --reporter-cli-verbose-output-limit <size in any unit>
  2. newman run <collection> --verbose --reporter-cli-no-verbose-output-limit

The --reporter-cli-verbose-output-limit uses bytes to convert string input to bytes. In case of any invalid input, a warning is shown and a fallback size of 2KB is used.

Copy link
Member

@shamasis shamasis left a comment

Choose a reason for hiding this comment

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

We do not need to introduce two parameters for this. In the grand scheme of things this is a small feature and one parameter should do. Here's what we can do:

  1. We ALWAYS clip. No matter what.
    Providing Infinity as an input option can then automatically prevent clipping with least complicated codes. (And we mention this behaviour in docs as a short byline.)

  2. We take only one option --reporter-cli-truncate-body-output (people will 99% chance refer documentation to get to this, hence we name accordingly.)

  3. In the documentation, mention example input such as '1024', '1KB', etc so that users know what to input here and not need to refer the "bytes" readme doc.

PS - apologies for slow review, we are busy with a tonne of year change activities.

@@ -26,7 +27,7 @@ var _ = require('lodash'),
process: 'process',
total: 'total'
},
BODY_CLIP_SIZE = 2048,
DEFAULT_VERBOSE_OUTPUT_TRUNCATION_LIMIT = '2KB',
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 keep the constant name to body clip size. This way there is no ambiguity as to what output it is clipping.

@@ -70,6 +71,8 @@ extractSNR = function (executions) {
* @param {Boolean=} reporterOptions.noFailures - Boolean flag to turn off failure reporting altogether, if set to true.
* @param {Boolean=} reporterOptions.noConsole - Boolean flag to turn off console logging, if set to true.
* @param {Boolean=} reporterOptions.noBanner - Boolean flag to turn off newman banner, if set to true.
* @param {String=} reporterOptions.verboseOutputLimit - Specify the truncation size of body in verbose CLI output.
Copy link
Member

Choose a reason for hiding this comment

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

We do not need to introduce two parameters for this. In the grand scheme of things this is a small feature and one parameter should do. Here's what we can do:

  1. We ALWAYS clip. No matter what.
  2. Providing Infinity as an input option can then automatically prevent clipping with least complicated codes. (And we mention this behaviour in docs as a short byline.)
  3. We take only one option --reporter-cli-truncate-body-output (people will 99% chance refer documentation to get to this, hence we name accordingly.)

Check for isNaN much earlier and prevent NaN from flowing too much down the code flow.


].join(` ${colors.gray(symbols.star)} `),
// eslint-disable-next-line max-len
bodyClipSize = reporterOptions.verboseOutputLimit ? bytes(reporterOptions.verboseOutputLimit) : bytes(DEFAULT_VERBOSE_OUTPUT_TRUNCATION_LIMIT);
Copy link
Member

Choose a reason for hiding this comment

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

Set the default value here. No need to compute. Instead at the below if block, set it if !isNaN and warn user in else of that block

reqText = reqText.substr(0, BODY_CLIP_SIZE) +
colors.brightWhite(`\n(showing ${util.filesize(BODY_CLIP_SIZE)}/${util.filesize(reqTextLen)})`);
// truncate very large request (if noVerboseOutputLimit flag is set, remove any limitations)
if (!reporterOptions.noVerboseOutputLimit && reqTextLen > bodyClipSize) {
Copy link
Member

Choose a reason for hiding this comment

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

If the above recommendations are followed, then this if block become almost as original.

@Sid200026
Copy link
Author

@shamasis, I have simplified the implementation by using a single flag.

newman run <collection> --verbose --reporter-cli-truncate-body-output <size in any unit>. You can pass infinite to remove any truncation limit.

@shamasis
Copy link
Member

@shamasis, I have simplified the implementation by using a single flag.

newman run <collection> --verbose --reporter-cli-truncate-body-output <size in any unit>. You can pass infinite to remove any truncation limit.

I think this is ready for approval. Feature now looks cool.

I've marked it as approved, but there's one tiny comment to resolve for you.

shamasis
shamasis previously approved these changes Jan 24, 2022
Copy link
Member

@shamasis shamasis left a comment

Choose a reason for hiding this comment

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

Approved. One comment to be resolved.


if (reporterOptions.truncateBodyOutput) {
// if infinite is passed to truncate-body-output
if (reporterOptions.truncateBodyOutput === 'infinite') {
Copy link
Member

Choose a reason for hiding this comment

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

Why not take input as "infinity"? (Lowercase compare to ensure we don't need case sensitivity)

If you don't like the word infinity, then can use "none" too.

Copy link
Author

Choose a reason for hiding this comment

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

I think infinity would be better since the description for --reporter-cli-truncate-body-output is specify the truncation size of body in verbose CLI output for requests and responses.

@dronky
Copy link

dronky commented Dec 12, 2022

Hello, this improvement is waiting for 1 year already, is there an eta when it'll be published or is there an alternative to avoid output limiting?

@jmarcher
Copy link

jmarcher commented Jul 6, 2023

I would love to have this

@egbastos666
Copy link

Hello,

Is there any forecast to have this option published on newman? That's really something that will help a lot everyone, have the full request / response body written into a log. We work with images base64 inside the body and is impossible to check if everything was sent as expected

Eduardo

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.55%. Comparing base (90f4449) to head (a66db04).
Report is 92 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2883      +/-   ##
===========================================
+ Coverage    90.96%   91.55%   +0.59%     
===========================================
  Files           21       21              
  Lines         1151     1161      +10     
  Branches       349      352       +3     
===========================================
+ Hits          1047     1063      +16     
+ Misses         104       98       -6     
Flag Coverage Δ
cli 82.77% <100.00%> (+0.67%) ⬆️
integration 40.74% <7.14%> (-0.27%) ⬇️
library 58.13% <7.14%> (-0.42%) ⬇️
unit 74.50% <7.14%> (-0.57%) ⬇️

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.

Verbose truncates output down to 2.05kB
9 participants