Skip to content

DAOS-18304 ddb: Add Go unit tests for ddb command layer#18086

Open
knard38 wants to merge 21 commits into
masterfrom
ckochhof/fix/master/daos-18304-patch-002
Open

DAOS-18304 ddb: Add Go unit tests for ddb command layer#18086
knard38 wants to merge 21 commits into
masterfrom
ckochhof/fix/master/daos-18304-patch-002

Conversation

@knard38

@knard38 knard38 commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Description

Overview

Go unit tests for the ddb CLI command layer, building on the build-tag CGo stub infrastructure
introduced in the predecessor PR #18124.

Tests are gated behind the test_stubs build tag and run without a live VOS environment. The CGo
function stubs provided by libddb_stubs.go expose configurable return-code variables
(ddb_init_RC, ddb_pool_is_open_RC, …) and hook variables (ddb_init_Fn, …) that let tests
control stub behaviour without modifying production code.

New Files

test_helpers.go

Test infrastructure shared across all test files:

  • newTestContext: allocates a DdbContext backed by the CGo stubs.
  • captureStdout: redirects os.Stdout to a pipe for the duration of a function call and
    returns the captured output as a string.
  • runCmdToStdout: calls parseOpts() with given args and captures stdout.
  • runMainFlow: simulates the full main() flow (parse → version check → run) without
    calling os.Exit().
  • assertContainsAll: asserts that a string contains every element of a given slice — used by
    TestManPage to avoid repetitive loops.
  • isArgEqual: type-safe argument comparison helper.
ddb_commands_test.go

Tests for the grumble command definitions:

  • TestHelpCmds: verifies --help output for the ls and open commands.
  • TestCmds: argument and option parsing for ls (--recursive, --details), open
    (--vos_path, --db_path, --write_mode), feature (--enable/--disable), dtx_aggr
    (mutual-exclusion validation), close, and version.
  • TestManPage: verifies man page output both to stdout and to a file, using a shared
    expSections list.
main_test.go

Tests for the CLI entry-point logic:

  • TestParseOpts: default option values, flag parsing (--debug, --write, --vos_path,
    --db_path, --version, --cmd, --cmd_file) and error cases (conflicting
    --cmd/--cmd_file, missing --vos_path when --db_path is set).
  • TestRun: version output, unknown command detection, noAutoOpen list (feature, open,
    smd_sync), auto-open via --vos_path/--db_path, ctx.Init() failure, ctx.Open() failure.
  • TestRunMultiLineCommandFile: verifies that a command file with multiple commands executes
    all of them in order.
  • TestStrToLogLevels: all supported log-level string conversions.
  • TestNewLogger: logger creation for each log level.
  • TestClosePoolIfOpen: pool-not-open, successful close, and close-error paths.

Steps for the author:

  • Commit message follows the guidelines.
  • Appropriate Features or Test-tag pragmas were used.
  • Appropriate Functional Test Stages were run.
  • At least two positive code reviews including at least one code owner from each category referenced in the PR.
  • Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

  • Gatekeeper requested (daos-gatekeeper added as a reviewer).

@knard38 knard38 self-assigned this Apr 22, 2026
@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-002 branch 2 times, most recently from 628036b to c58c604 Compare April 22, 2026 14:29
@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-001 branch from 3c4faba to 24d96e6 Compare April 22, 2026 14:30
@github-actions

github-actions Bot commented Apr 22, 2026

Copy link
Copy Markdown

Ticket title is 'Add unit test to ddb go code'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-18304

@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-001 branch from 24d96e6 to f81f3be Compare April 23, 2026 05:02
@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-002 branch 3 times, most recently from 62c66ea to 6e01e12 Compare April 23, 2026 08:20
@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-002 branch from 6e01e12 to cda7452 Compare May 11, 2026 09:10
@daosbuild3

Copy link
Copy Markdown
Collaborator

@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-002 branch from 47f06b5 to a511b14 Compare May 11, 2026 10:36
@daosbuild3

Copy link
Copy Markdown
Collaborator

@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-002 branch from a511b14 to 03189b1 Compare May 11, 2026 12:35
@knard38 knard38 changed the base branch from ckochhof/fix/master/daos-18304-patch-001 to ckochhof/fix/master/daos-18304-patch-001-split May 11, 2026 12:36
@daosbuild3

Copy link
Copy Markdown
Collaborator

@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-002 branch 2 times, most recently from c3dd676 to bd9f1f2 Compare May 11, 2026 15:22
@daosbuild3

Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18086/17/execution/node/717/log

Base automatically changed from ckochhof/fix/master/daos-18304-patch-001-split to master May 18, 2026 14:43
@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-002 branch 3 times, most recently from f6a67bd to 8b2e76b Compare May 19, 2026 09:33
Introduce the Go test suite for the ddb CLI layer, built on top of the
build-tag CGo stub infrastructure landed in #18124:

- Add test_helpers.go: newTestContext(t) resets all CGo stubs via
  resetDdbStubs() and returns a *DdbContext ready for use in tests.
  Test cases set per-function _Fn hook variables directly.
- All test files carry the //go:build test_stubs tag so they only
  compile when the stub infrastructure is present.
- TestCmds: open (default, write_mode, db_path), feature (show, enable,
  disable), and dtx_aggr (mutual exclusion, cmt_time, cmt_date, path).
  Adds skipCmdLine field for flags shared between CLI and grumble layers.
- TestHelpCmds: unknown-command help flow.
- TestParseOpts / TestRun: CLI-level option parsing and run() dispatch,
  including unknown-command detection for both command-line and
  command-file paths.
- TestNewLogger: 6 sub-cases (default level, explicit debug, invalid
  level, valid LogDir, non-existent LogDir, LogDir is a file).
- TestClosePoolIfOpen: Close not called when already closed, called when
  open, Close error tolerated.

Test-tag: unittest
Required-githooks: yes
Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
@knard38 knard38 force-pushed the ckochhof/fix/master/daos-18304-patch-002 branch from 8b2e76b to 658f2f3 Compare May 19, 2026 10:18
@daosbuild3

Copy link
Copy Markdown
Collaborator

@knard38 knard38 requested review from mjmac and tanabarr May 19, 2026 12:11
…pers

Per reviewer feedback (r3318912192): replace the hand-rolled isArgEqual
helper with the existing test.CmpAny utility. test.CmpAny uses cmp.Diff
for clearer failure output and calls t.Fatalf directly, removing the
error propagation boilerplate from stub closures.

To flow the subtest t into the stub closures, setup fields in TestRun and
TestCmds are changed from func() to func(*testing.T), and named helper
factories (openFnChecking, openFnCheckingWriteMode, lsFnChecking,
featureFnCheckingShow, dtxAggrFnChecking) gain a leading t *testing.T
parameter. The isArgEqual function and its unused reflect import are
removed from test_helpers.go.

Since test.CmpAny calls t.Fatalf (which triggers runtime.Goexit),
captureStdout is updated to use named return values and move w.Close()
and <-done into the defer, ensuring the pipe and goroutine are always
cleaned up even when the test fails mid-stub.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
@daosbuild3

Copy link
Copy Markdown
Collaborator

kanard38 added 6 commits June 1, 2026 07:11
Per reviewer feedback (r3318906385): remove the runMainFlow helper and
inline captureStdout(func() error { return runDdb(ctx, args) }) directly
at the 8 call sites in main_test.go and ddb_commands_test.go.

runMainFlow was a meaningful abstraction when it contained ~15 lines of
logic (parseOpts, version handling, run). After the runDdb extraction
commit it became a thin 3-line pass-through that adds indirection without
semantic value. Inlining makes each test site self-contained and explicit
about what it does: capture stdout while running the production entry point.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
…Contains

Per reviewer feedback (r3318926829): assertContainsAll has no ddb-specific
logic and belongs in the shared test utilities package so it can be reused
across the entire control-plane test suite.

Add AssertStringContains(t, s string, substrings ...string) to
common/test/utils.go. The variadic signature handles both single and
multiple substring checks; existing slice call sites use the spread
operator (expSections...).

Remove assertContainsAll from test_helpers.go along with the now-unused
fmt, strings, and common/test imports.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Per reviewer feedback (r3318965302): split the single TestRun function
into two focused test functions.

Previously TestRun ran each case in two modes (command-line and
command-file) controlled by optional cmdFileArgs/cmdFileCmd struct
fields and a conditional t.Run block. This made the table struct heavier
and the runner harder to follow.

After the split:
- TestRun covers command-line mode only; the table has a flat
  (args, setup, expStdout, expErr) struct and a single straightforward
  runner body.
- TestRunCommandFile covers command-file mode only; the table uses a
  (flags, cmdLine, setup, expStdout, expErr) struct that matches the
  command-file execution model directly.

The five openFn* helper factories are moved to package level so both
test functions can share them without duplication.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Per reviewer tip (r3318987058): follow the Test<PkgName>_<funcName>
convention used in cmd/daos and other control-plane packages, so that
failing tests are immediately identifiable by package when running the
full unit test suite.

Rename map (12 functions, 3 files):
  TestParseOpts              -> TestDdb_parseOpts
  TestRun                   -> TestDdb_runDdb
  TestRunCommandFile        -> TestDdb_runDdbCommandFile
  TestRunMultiLineCommandFile -> TestDdb_runFileCmds
  TestStrToLogLevels        -> TestDdb_strToLogLevels
  TestNewLogger             -> TestDdb_newLogger
  TestClosePoolIfOpen       -> TestDdb_closePoolIfOpen
  TestHelpCmds              -> TestDdb_HelpCmds
  TestCmds                  -> TestDdb_Cmds
  TestManPage               -> TestDdb_ManPage
  TestListVosFiles          -> TestDdb_listVosFiles
  TestFilterSuggestions     -> TestDdb_filterSuggestions

No logic changes.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
The runHelpCmd helper was called in a single place with straightforward
logic. Inlining it into the t.Run body makes TestDdb_HelpCmds fully
self-contained without unnecessary indirection.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
@knard38 knard38 requested a review from kjacque June 1, 2026 08:36
@daosbuild3

Copy link
Copy Markdown
Collaborator

@daosbuild3

Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18086/35/testReport/

kanard38 added 2 commits June 2, 2026 09:01
The helper was silently converting errHelpRequested to nil, hiding a
sentinel that runDdb acts on. Unit tests should verify that parseOpts
returns the sentinel when --help is passed.

Remove the masking guard from runCmdToStdout and add expErr:
errHelpRequested to the two help-message test cases. Move the expStdout
check before the early-return guard so these cases still verify both the
sentinel and the stdout output.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
@daosbuild3

Copy link
Copy Markdown
Collaborator

knard38 and others added 2 commits June 2, 2026 12:48
Replace fmt.Println("Open called") in openFnChecking and
openFnCheckingWriteMode with a *bool pointer parameter. Each test case
declares a closure-scoped "called" bool, passes &called to the stub,
and registers t.Cleanup to assert invocation. The same pattern is applied
to TestDdb_runFileCmds which used fmt.Println in ls/version stubs.

This removes the dependency on captureStdout for stub verification and
eliminates the risk of false positives from production code printing the
same string. The remaining captureStdout usages (version output and help
text) are legitimate: grumble writes directly to os.Stdout with no
injection point.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
@daosbuild3

Copy link
Copy Markdown
Collaborator

@kjacque kjacque left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks!

@knard38

knard38 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

LGTM. Thanks!

Thanks to your review which allowed to really improve the PR :-)

@tanabarr tanabarr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

if err != nil {
exitWithError(errors.Wrap(err, loggerInitErr))
var log *logging.LeveledLogger
if log, err = newLogger(opts); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Rather than declaring log explicitly, I would personally just use a different err variable name to avoid shadowing: if log, errIn := newLogger(opts); ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This solution is not possible as log will be out of scope.
However, the following solution seems to be indeed more idiomatic

log, err := newLogger(opts)
if err != nil {
   ...
}

If this solution is OK for you, I will integrate it in the follow-up PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed with commit 04cdf46 of the PR #18466

@tanabarr tanabarr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm personally not too worried about shadowing within an if statement, we know that using short variable declaration will result in a new variable scoped within the if statement.

@daosbuild3

Copy link
Copy Markdown
Collaborator

@daosbuild3

Copy link
Copy Markdown
Collaborator

@daosbuild3

Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18086/41/testReport/

@knard38

knard38 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

All CI failures in build #41 are pre-existing infrastructure or unrelated issues:

@knard38 knard38 requested a review from a team June 9, 2026 08:59
@knard38

knard38 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@daos-stack/daos-gatekeeper according to my previous comment, could you land this PR with the following commit:

  • title: DAOS-18304 ddb: add Go unit tests for ddb command layer
  • body:
Introduce the Go test suite for the ddb CLI layer, built on top of the
build-tag CGo stub infrastructure introduced with commit 86f31a2bd3:

- Add test_helpers.go: newTestContext(t) resets all CGo stubs via
  resetDdbStubs() and returns a *DdbContext ready for use in tests.
  Test cases set per-function _Fn hook variables directly.
- All test files carry the //go:build test_stubs tag so they only
  compile when the stub infrastructure is present.
- TestDdb_Cmds: open (default, write_mode, db_path), feature (show,
  enable, disable), and dtx_aggr (mutual exclusion, cmt_time, cmt_date,
  path).
- TestDdb_HelpCmds: --help output for the ls and open commands.
- TestDdb_parseOpts / TestDdb_runDdb / TestDdb_runDdbCommandFile:
  CLI-level option parsing and runDdb() dispatch, including
  unknown-command detection for both command-line and command-file paths.
- TestDdb_runFileCmds: verifies that a multi-line command file executes
  all commands in order.
- TestDdb_newLogger: 6 sub-cases (default level, explicit debug, invalid
  level, valid LogDir, non-existent LogDir, LogDir is a file).
- TestDdb_closePoolIfOpen: Close not called when already closed, called
  when open, Close error tolerated.

Extract runDdb(ctx, args) as a production function containing the core
execution logic so that tests exercise the real code path without
triggering os.Exit. main() retains only OS-level setup (SetTraceback,
DisableCStdoutBuffering).

Add AssertStringContains(t, s string, substrings ...string) to
common/test/utils.go for reuse across the control-plane test suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants