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

Cli percentile column fit #354

Merged
merged 4 commits into from
Jun 12, 2020
Merged

Conversation

oschaaf
Copy link
Member

@oschaaf oschaaf commented Jun 10, 2020

  • Cap the number of decimals when rendering the percentiles column data.
  • Add a more detailed test for fixating output of the human readable output formatter, so we'll catch unintended changes better next time.

Fixes #352

Signed-off-by: Otto van der Schaaf [email protected]

@oschaaf oschaaf added P1 waiting-for-review A PR waiting for a review. labels Jun 10, 2020
@@ -192,6 +192,13 @@ TEST_F(MediumOutputCollectorTest, FortioFormatter) {
"test/test_data/output_formatter.medium.fortio.gold");
}

TEST_F(MediumOutputCollectorTest, ConsoleOutputFormatter) {
const auto input_proto = loadProtoFromFile("test/test_data/large-sample.json");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any way we can minimize this testdata file so that it only contains the minimum possible for the test to make sense? It is generally easy to add testdata files by just copying some actual production file, but it tends to hurt on maintenance as other test maintainers don't know which portion of the file is relevant to the intention of the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good point, that was suboptimal. I pushed 0711c15 to improve, which makes this specific to the problem I'm trying to address here.

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jun 10, 2020
@oschaaf oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jun 10, 2020
Copy link
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

Thank you for minimizing the testdata.

@mum4k mum4k merged commit 1fa1cde into envoyproxy:master Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 waiting-for-review A PR waiting for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Human readable output histogram: 1st column too small
2 participants