diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 332becd..8169b8d 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -5,6 +5,7 @@ import ( "bytes" "errors" "fmt" + "io" "math/rand" "os" "sync" @@ -14,6 +15,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" urfcli "github.com/urfave/cli/v2" + "sneak.berlin/go/mfer/internal/log" "sneak.berlin/go/mfer/mfer" ) @@ -27,6 +29,7 @@ const ( testManifest = "/manifest.mf" testFlagBase = "--base" testFlagNoExtra = "--no-extra-files" + testFlagVersion = "--version" ) var errSimulatedWrite = errors.New("simulated write failure") @@ -39,12 +42,20 @@ var errSimulatedWrite = errors.New("simulated write failure") var runMu sync.Mutex // runCLI invokes RunWithOptions while holding runMu so parallel tests -// capture their own output. +// capture their own output. Before releasing the lock it points the +// process-global logger at io.Discard: other tests log outside the lock +// (manifest loads, scans), and those lines must not land in this run's +// buffers once it has returned and its test is reading them. func runCLI(opts *RunOptions) int { runMu.Lock() defer runMu.Unlock() - return RunWithOptions(opts) + exitCode := RunWithOptions(opts) + + log.SetOutput(io.Discard, io.Discard) + log.Init() + + return exitCode } func TestMain(m *testing.M) { @@ -102,7 +113,7 @@ func TestVersionCommand(t *testing.T) { t.Parallel() fs := afero.NewMemMapFs() - opts := testOpts([]string{testApp, "version"}, fs) + opts := testOpts([]string{testApp, cmdVersion}, fs) exitCode := runCLI(opts) @@ -113,6 +124,117 @@ func TestVersionCommand(t *testing.T) { assert.Contains(t, stdout, "abc123") } +// TestVFlagCollision covers the -v/--verbose vs --version flag interaction +// (issue #64). Verbose owns -v; version answers to --version and -V. None of +// these invocations may produce a parser error, and the two ways of asking +// for the version must print the same thing. +func TestVFlagCollision(t *testing.T) { + t.Parallel() + + // Invocations that must print the version and exit 0. + versionCases := map[string][]string{ + "long version flag": {testApp, testFlagVersion}, + "short version flag": {testApp, "-V"}, + "verbose then version": {testApp, "-v", testFlagVersion}, + "long verbose and version": {testApp, "--verbose", testFlagVersion}, + } + + for name, args := range versionCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + opts := testOpts(args, afero.NewMemMapFs()) + exitCode := runCLI(opts) + + assert.Equal(t, 0, exitCode, "stderr: %s", testStderr(t, opts)) + assert.Contains(t, testStdout(t, opts), mfer.Version) + assert.NotContains(t, testStderr(t, opts), "two forms of the same flag") + }) + } + + // Invocations that must enable verbose and exit 0 without a parser error. + verboseCases := map[string][]string{ + "short verbose flag": {testApp, "-v"}, + "long verbose flag": {testApp, "--verbose"}, + } + + for name, args := range verboseCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + opts := testOpts(args, afero.NewMemMapFs()) + exitCode := runCLI(opts) + + assert.Equal(t, 0, exitCode, "stderr: %s", testStderr(t, opts)) + assert.Contains(t, testStdout(t, opts), cmdGenerate, + "root should show help listing subcommands") + }) + } +} + +// TestVersionFlagAndCommandMatch asserts that "mfer --version" and +// "mfer version" produce identical output (issue #64). +func TestVersionFlagAndCommandMatch(t *testing.T) { + t.Parallel() + + flagOpts := testOpts([]string{testApp, testFlagVersion}, afero.NewMemMapFs()) + require.Equal(t, 0, runCLI(flagOpts)) + + cmdOpts := testOpts([]string{testApp, cmdVersion}, afero.NewMemMapFs()) + require.Equal(t, 0, runCLI(cmdOpts)) + + assert.Equal(t, testStdout(t, flagOpts), testStdout(t, cmdOpts)) +} + +// TestRootVerbosityFlags asserts that -q and -v given before any subcommand +// name take effect: in the root itself, and in the fetch and export +// subcommands (issue #64). It does not run in parallel: other tests log +// outside runMu (manifest loads, scans), and a line logged while one of these +// runs is in progress lands in its output. +// +//nolint:paralleltest // see above +func TestRootVerbosityFlags(t *testing.T) { + t.Run("quiet with no subcommand hides the banner", func(t *testing.T) { + loud := testOpts([]string{testApp}, afero.NewMemMapFs()) + require.Equal(t, 0, runCLI(loud)) + assert.Contains(t, testStdout(t, loud), banner) + + quiet := testOpts([]string{testApp, "-q"}, afero.NewMemMapFs()) + require.Equal(t, 0, runCLI(quiet)) + assert.Contains(t, testStdout(t, quiet), cmdGenerate) + assert.NotContains(t, testStdout(t, quiet), banner) + }) + + t.Run("quiet before fetch hides the banner", func(t *testing.T) { + opts := testOpts([]string{testApp, "-q", cmdFetch}, afero.NewMemMapFs()) + + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), errURLRequired.Error()) + assert.NotContains(t, testStdout(t, opts), banner) + }) + + t.Run("verbose twice before fetch enables debug logging", func(t *testing.T) { + opts := testOpts([]string{testApp, "-v", "-v", cmdFetch}, afero.NewMemMapFs()) + + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), "fetchManifestOperation()") + }) + + t.Run("quiet before export hides the manifest summary", func(t *testing.T) { + fs := afero.NewMemMapFs() + manifest := buildTestManifest(t, map[string][]byte{"a.txt": []byte("a")}) + require.NoError(t, afero.WriteFile(fs, testManifest, manifest, 0o644)) + + loud := testOpts([]string{testApp, cmdExport, testManifest}, fs) + require.Equal(t, 0, runCLI(loud)) + assert.Contains(t, testStderr(t, loud), "loaded manifest") + + quiet := testOpts([]string{testApp, "-q", cmdExport, testManifest}, fs) + require.Equal(t, 0, runCLI(quiet)) + assert.NotContains(t, testStderr(t, quiet), "loaded manifest") + }) +} + func TestHelpCommand(t *testing.T) { t.Parallel() diff --git a/internal/cli/mfer.go b/internal/cli/mfer.go index 8c4e769..ccd878a 100644 --- a/internal/cli/mfer.go +++ b/internal/cli/mfer.go @@ -19,6 +19,7 @@ const ( cmdCheck = "check" cmdExport = "export" cmdFetch = "fetch" + cmdVersion = "version" flagProgress = "progress" @@ -68,6 +69,12 @@ func (mfa *CLIApp) VersionString() string { return mfer.Version } +// printVersion writes the version line shared by the --version flag and the +// version subcommand, so both produce identical output. +func (mfa *CLIApp) printVersion() { + _, _ = fmt.Fprintf(mfa.Stdout, "%s version %s\n", mfa.appname, mfa.VersionString()) +} + func (mfa *CLIApp) printBanner() { if log.GetLevel() <= log.InfoLevel { _, _ = fmt.Fprintln(mfa.Stdout, banner) @@ -78,20 +85,34 @@ func (mfa *CLIApp) printBanner() { } } +// setVerbosity sets the log level from -v and -q, given before the subcommand +// name (the root's copies), after it (the subcommand's own copies), or both. +// urfave/cli reads a flag from the nearest command that defines it, so each +// command in the lineage is asked. The highest -v count wins rather than the +// sum, because a subcommand without its own copies reads the root's. func (mfa *CLIApp) setVerbosity(c *cli.Context) { _, present := os.LookupEnv("MFER_DEBUG") + verbosity := 0 + quiet := false + + for _, ctx := range c.Lineage() { + verbosity = max(verbosity, ctx.Count("verbose")) + quiet = quiet || ctx.Bool("quiet") + } + switch { case present: log.EnableDebugLogging() - case c.Bool("quiet"): + case quiet: log.SetLevel(log.ErrorLevel) default: - log.SetLevelFromVerbosity(c.Count("verbose")) + log.SetLevelFromVerbosity(verbosity) } } -// commonFlags returns the flags shared by most commands (-v, -q) +// commonFlags returns the -v and -q flags taken by the root and by the +// generate, check, freshen and fetch subcommands. func commonFlags() []cli.Flag { return []cli.Flag{ &cli.BoolFlag{ @@ -259,6 +280,8 @@ func (mfa *CLIApp) exportCommand() *cli.Command { Usage: "Export manifest contents as JSON", ArgsUsage: "[manifest file or URL]", Action: func(c *cli.Context) error { + mfa.setVerbosity(c) + return mfa.exportManifestOperation(c) }, } @@ -266,10 +289,10 @@ func (mfa *CLIApp) exportCommand() *cli.Command { func (mfa *CLIApp) versionCommand() *cli.Command { return &cli.Command{ - Name: "version", + Name: cmdVersion, Usage: "Show version", Action: func(_ *cli.Context) error { - _, _ = fmt.Fprintln(mfa.Stdout, mfa.VersionString()) + mfa.printVersion() return nil }, @@ -325,6 +348,21 @@ func (mfa *CLIApp) run(args []string) { log.SetOutput(mfa.Stdout, mfa.Stderr) log.Init() + // -v means verbose, not version. urfave/cli's built-in version flag + // claims -v by default, which made "mfer -v --version" fail to parse and + // gave -v a different meaning at the root than on the generate, check, + // freshen and fetch subcommands, where it means verbose. Verbose is the + // more common meaning of -v in tools that offer both, so -v means verbose + // at the root too and the version flag takes the capital -V. + // VersionFlag and VersionPrinter are urfave/cli package globals; run() is + // serialized in tests, so assigning them here is safe. + cli.VersionFlag = &cli.BoolFlag{ + Name: cmdVersion, + Aliases: []string{"V"}, + Usage: "print the version", + } + cli.VersionPrinter = func(_ *cli.Context) { mfa.printVersion() } + mfa.app = &cli.App{ Name: mfa.appname, Usage: "Manifest generator", @@ -332,11 +370,13 @@ func (mfa *CLIApp) run(args []string) { EnableBashCompletion: true, Writer: mfa.Stdout, ErrWriter: mfa.Stderr, + Flags: commonFlags(), Action: func(c *cli.Context) error { if c.Args().Len() > 0 { return fmt.Errorf("%w %q", errUnknownCommand, c.Args().First()) } + mfa.setVerbosity(c) mfa.printBanner() return cli.ShowAppHelp(c)