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

Rework env-var interface to set log streams from unit-tests that exercise print diagnostics code. #534

Open
gapisback opened this issue Jan 18, 2023 · 3 comments
Assignees
Labels
code-cleanup Minor corrections, improvements, code-cleanup; not necessarily a bug per se. enhancement New feature or request

Comments

@gapisback
Copy link
Collaborator

gapisback commented Jan 18, 2023

Under PR #514, couple of new interfaces have been added for use (mainly) by unit-tests to set the output log streams. These functions are set_log_streams_for_tests(), and set_log_streams_for_error_tests().

These set methods redirect outputs to stdout and stderr if env-var VERBOSE is set (1).

There are couple of unit-test cases which exercise print diagnostic functions. E.g.

Both these test cases redirect output using two different interfaces: set_log_streams_for_tests() and set_log_streams_for_error_tests(). That's a minor mis-use of these interfaces.

The real issue is that output from test cases like test_btree_print_diags() becomes very huge. It's over 6M usually. This causes CI browsers to become unwieldy when looking at such outputs.

The things we need to resolve under this issue are:

  1. Rework the interfaces so that when these tests are run in CI (which currently invokes stuff with VERBOSE=1) that none of this output is generated to stdout or stderr (causing browsers to die).

  2. Provide another way for people to be able to run these tests on their own dev machines and collect the outputs from stdout. Perhaps, we could consider different values for the env-var which will set up the desired log stream handles.

  3. Consider adding a new set_log_streams_for_diagnostic_tests() interface. Rework the few unit-tests that are currently exercising these print diagnostics to use this interface (instead).

  4. When the support for VERBOSE=1 was added (under PR Simplify output handling in unit tests #494), the general agreement from reviewers was that it would be better to have --cmd-line-flags to support different verbose print options, rather than this env-var. Review that approach and come up with a solution to address all usages / requirements.

In that last PR #494, following note was recorded :


Some time ago, I had added this --verbose-progress option that was used initially in functional/splinter_test.c to do some verbose progress reporting while large test workloads run. This arg is parsed and tracked as a bool in test_exec_config->verbose_progress.

With some work we could co-opt this flag to also mean what this VERBOSE=1 env-var wants to do. This boolean field may be the replacement for your newly-added Ctest_verbosity flag.


Here are some cmdline options to consider adding:

  • --verbose-progress: Current interface, to print verbose messages from functional tests
  • --verbose-messages: New interface, to have errors & other messages from unit-tests being printed
  • --verbose-diagnostics: New interface, to have outputs from diagnostic tests come to stdout

Depending on how we agree to reconcile these options, update the newly added set _log_streams_for*() interfaces accordingly.

@gapisback gapisback self-assigned this Jan 18, 2023
@gapisback gapisback added code-cleanup Minor corrections, improvements, code-cleanup; not necessarily a bug per se. enhancement New feature or request labels Jan 18, 2023
@rosenhouse
Copy link
Member

Please consider re-titling this issue to "Test output should never be more than 100k lines long" or "CI viewer should not crash on test output" or something along those lines.

All the rest of your discussion here is, to me, secondary to ensuring that CI can provide us the complete output of tests.

@gapisback
Copy link
Collaborator Author

Brainstormed this with @rosenhouse : Here's a summary approach we sort-of 'settled on'.

  • Try to model these output verbosity levels on the lines of what's being done for syslog error levels.
  • Postgres has similar message levels, which are mapped to syslogs' levels.

Suggested interfaces:

unit/misc_test --verbosity={ <level> | "silent" | "error" | "debug" }

Levels: silent=0; error=3; debug=7

Consider (if needed) adding something like silence_log_streams_for_tests() to clear-out log-streams for tests that want to go to silent output behaviour.

@rosenhouse
Copy link
Member

We also discussed, as an intermediate step, just sticking with the existing VERBOSE= environment variable a bit longer, so we could capture the semantics without doing to work to introduce the new command-line flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-cleanup Minor corrections, improvements, code-cleanup; not necessarily a bug per se. enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants