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

Refactor progress bar & summary line logic #2984

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

terrelln
Copy link
Contributor

@terrelln terrelln commented Jan 7, 2022

  • Centralize the logic about whether to print the progress bar or not in
    the *_PROGRESS() macros.
  • Centralize the logc about whether to print the summary line or not in
    FIO_shouldDisplayFileSummary() and
    FIO_shouldDisplayMultipleFileSummary().
  • Make --progress work for non-zstd (de)compressors.
  • Clean up several edge cases in compression and decompression progress
    printing along the way. E.g. wrong log level, or missing summary line.

One thing I don't like about stdout mode, which sets the display level
to 1, is that warnings aren't displayed. After this PR, we could change
stdout mode from lowering the display level, to defaulting to implied
--no-progress. But, I think that deserves a separate PR.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jan 7, 2022

That's much clearer ! Thanks for the improvement !

One thing I don't like about stdout mode, which sets the display level to 1, is that warnings aren't displayed.

This corresponds to a zstdcat scenario :
we want to avoid a situation where the output, presumed the console,
intermix content and warning messages.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jan 7, 2022

Btw, do we have tests that check the presence or absence of notification / progress / warning messages ?
This could prove helpful, to ensure that some display properties are preserved across future modifications.

@terrelln
Copy link
Contributor Author

terrelln commented Jan 8, 2022

Btw, do we have tests that check the presence or absence of notification / progress / warning messages ?
This could prove helpful, to ensure that some display properties are preserved across future modifications.

I agree, there are a lot of test cases that I ran manually that I would like to include in our test suite. playTests.sh isn't really set up to test the stderr output matches some expected output (with wildcards / regexes) in a sane way. Because there are tens of scenarios we'd want to test.

I'd rather have some way to tell the test suite to run a command, and check that the stderr output matches some file. There are definitely pre-built tools that can do this. So I'm going to spend a little bit of time trying to set it up.

@terrelln
Copy link
Contributor Author

I'm writing a small tool to test running the CLI & checking stderr/stdout/exit code match some expectations.

I will submit the tool as a PR, then rebase this on top of it with the test cases.

We've been unable to effectively test cases where stdin/stdout/stderr
are consoles, because in our test cases they generally aren't. Allow the
command line flags `--fake-std{in,out,err}-is-console` to tell the CLI
to pretend that std{in,out,err} is a console.
@terrelln terrelln force-pushed the decompress-progress branch 2 times, most recently from 03a2c52 to 26c3e42 Compare December 14, 2022 22:03
* Centralize the logic about whether to print the progress bar or not in
  the `*_PROGRESS()` macros.
* Centralize the logc about whether to print the summary line or not in
  `FIO_shouldDisplayFileSummary()` and
  `FIO_shouldDisplayMultipleFileSummary()`.
* Make `--progress` work for non-zstd (de)compressors.
* Clean up several edge cases in compression and decompression progress
  printing along the way. E.g. wrong log level, or missing summary line.

One thing I don't like about stdout mode, which sets the display level
to 1, is that warnings aren't displayed. After this PR, we could change
stdout mode from lowering the display level, to defaulting to implied
`--no-progress`. But, I think that deserves a separate PR.
@terrelln terrelln merged commit fbff782 into facebook:dev Dec 14, 2022
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.

3 participants