vmware / splinterdb

High Performance Embedded Key-Value Store
https://splinterdb.org
Apache License 2.0
680 stars 57 forks source link

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

Open gapisback opened 1 year ago

gapisback commented 1 year ago

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 #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:

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

rosenhouse commented 1 year ago

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 commented 1 year ago

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

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 commented 1 year ago

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.