microsoft / microsoft/go-sqlcmd

Production code imports `testing` package (`internal/config/config.go`, `internal/cmdparser/test.go`)

Open
#780 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
595
Forks
91
Avg merge
9h 35m
Merged PRs (30d)
1

Description

Summary

internal/config/config.go and internal/cmdparser/test.go are non-_test.go files that import the standard library testing package, which is a Go anti-pattern — it pulls test-only code (and its transitive deps) into production builds.

Noted by @shueybubbles in https://github.com/microsoft/go-sqlcmd/pull/747#discussion_r3443879059.

Details

  • internal/config/config.go imports testing solely for SetFileNameForTest(t *testing.T), which only ever calls t.Name().
  • internal/cmdparser/test.go imports testing for TestSetup(t *testing.T) and related helpers.

Both files exist to provide test helpers callable from _test.go files in other packages, which is why they couldn't simply be renamed _test.go. However, the dependency on testing from production-compiled files is still undesirable.

Introduced in commit 10ee238 ("Modern CLI - Part 2"), long predating PR #747.

Suggested fix

Minimal-impact option:

  • Change config.SetFileNameForTest to accept a string (the test name) instead of *testing.T. Callers pass t.Name(). Drops the testing import from config.go.
  • Apply a similar refactor to cmdparser/test.go, or move its helpers into a dedicated cmdparsertest subpackage.

Larger option:

  • Move all cross-package test helpers into dedicated *test subpackages (e.g. internal/config/configtest, internal/cmdparser/cmdparsertest).

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading internal/config/config.go and internal/cmdparser/test.go, then inspect their cross-package test-helper callers. Refactor the helpers so production-compiled files no longer import Go's testing package, choosing between string-based inputs and dedicated test subpackages. Done means those imports are gone and the existing tests still pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.