Hey Bikram, thanks for taking the time to improve test coverage for cmd/api — going from 40.5% to 81% is a meaningful jump, and the TestRun_GracefulShutdown test in particular adds genuine value by being the only test that exercises the Run() function directly. Before we can merge this, I will need you to make a couple changes:

Critical

1. os.Exit(1) inside a test function will kill the entire test suite (cmd/api/app_test.go:549-553). Calling os.Exit(1) from test code terminates the entire go test process — no subsequent tests run, no defer cleanup executes, and no coverage data is written. This is especially ironic for a PR whose purpose is improving coverage. Replace with require.NoError:

   // Instead of the os.Exit(1) block:
   jsonConfig, err := appconf.LoadFromFile("../../config.example.json")
   require.NoError(t, err, "failed to load config.example.json")

Important

2. Unchecked os.Pipe() error (cmd/api/app_test.go:577). If os.Pipe() fails (e.g., file descriptor exhaustion in CI), both r and w are nil, and os.Stdout = w would cause a nil-pointer panic inside dumpConfigJSON. Check the error:

   r, w, err := os.Pipe()
   require.NoError(t, err, "failed to create OS pipe")

3. Stdout restoration should use defer (cmd/api/app_test.go:576-583). If dumpConfigJSON panics (note: the production code at app.go:254 contains its own os.Exit(1) on marshal failure), os.Stdout is never restored. Wrap the restoration in a defer:

   oldStdout := os.Stdout
   defer func() { os.Stdout = oldStdout }()

4. Unsafe type assertions will panic instead of producing useful test failures (cmd/api/app_test.go:597-622). Every bare .(float64), .(string), .(map[string]interface{}) assertion will panic with an unhelpful stack trace if the JSON structure changes. Use testify assertions instead, which is also consistent with the rest of the file:

   // Instead of:
   if parsed["port"].(float64) != 4000 {
       t.Fatalf("expected port 4000")
   }
   
   // Use:
   assert.Equal(t, float64(cfg.Port), parsed["port"])
   assert.Equal(t, "development", parsed["env"])

5. Assertions are hardcoded against config.example.json values (cmd/api/app_test.go:597-624). If anyone updates the example config (e.g., switches transit agencies), this test breaks for reasons unrelated to dumpConfigJSON. Assert against the values you loaded from the config instead of hardcoded strings:

   assert.Equal(t, float64(cfg.Port), parsed["port"])
   assert.Equal(t, gtfsCfg.GtfsURL, staticFeed["url"])
   assert.Equal(t, gtfsCfg.GTFSDataPath, parsed["data-path"])

6. No test coverage for auth header redaction — the dumpConfigJSON function contains security-sensitive logic that replaces auth header values with ***REDACTED*** (app.go:207-209, app.go:234-236). Since config.example.json has empty auth values, this path is never exercised. Consider adding a test case with populated auth credentials that verifies the secret does not appear in the output.

Fit and Finish

7. Missing trailing newline at end of file (cmd/api/app_test.go:625). The diff shows \ No newline at end of file. Add a newline after the final }.

8. Extra blank line after function signature (cmd/api/app_test.go:546) and double blank line (cmd/api/app_test.go:595-596). Minor style cleanup.

9. Use testify consistently — the test uses raw if/t.Fatalf for assertions while the rest of the file uses assert.Equal/require.NoError. Switching to testify would make the style consistent.

10. Field-by-field struct copy (cmd/api/app_test.go:560-573) — the GtfsConfigData to gtfs.Config conversion is duplicated in three places (main.go:67-80, app_test.go:484-494, and this new test). This is an existing issue in the codebase, not something you introduced, but if you're motivated, extracting a helper function in cmd/api/app.go would DRY up all three call sites. (There's an import cycle that prevents putting it in appconf, which is why it's done this way.)

Thanks again, and I look forward to merging this change.

Hey Bikram, good progress — you've knocked out the critical os.Exit issue, the pipe error handling, the defer for stdout restoration, the trailing newline, and the gtfsConfigFromData helper (which cleans up three call sites nicely). A few items from the previous round still need attention before we can merge:

Important

1. Unsafe type assertions still present (app_test.go:572, 575, 578)

This was item #4 from the last review. Bare type assertions like parsed["gtfs-static-feed"].(map[string]interface{}) will panic with an unhelpful stack trace if the JSON structure changes. Use two-value form or testify:

// Instead of:
staticFeed := parsed["gtfs-static-feed"].(map[string]interface{})

// Use:
staticFeed, ok := parsed["gtfs-static-feed"].(map[string]interface{})
require.True(t, ok, "gtfs-static-feed should be a map")

Same for parsed["gtfs-rt-feeds"].([]interface{}) on line 575 and feeds[0].(map[string]interface{}) on line 578.

2. No test for auth header redaction (app.go:227-229, 253-255)

This was item #6 from the last review. The dumpConfigJSON function contains security-sensitive logic that replaces auth header values with ***REDACTED***. Since config.example.json has empty auth values, this code path is never exercised by the test. Add a test case that populates auth credentials and verifies the secret doesn't appear in the output:

func TestDumpConfigJSON_RedactsAuthHeaders(t *testing.T) {
    cfg := appconf.Config{Port: 4000, Env: appconf.Development}
    gtfsCfg := gtfs.Config{
        GtfsURL:                 "https://example.com/gtfs.zip",
        StaticAuthHeaderKey:     "Authorization",
        StaticAuthHeaderValue:   "Bearer secret-token-123",
        RealTimeAuthHeaderKey:   "X-API-Key",
        RealTimeAuthHeaderValue: "my-secret-api-key",
        TripUpdatesURL:          "https://example.com/trips.pb",
    }

    // capture stdout, call dumpConfigJSON, parse output...

    assert.NotContains(t, output, "secret-token-123")
    assert.NotContains(t, output, "my-secret-api-key")
    assert.Contains(t, output, "***REDACTED***")
}

3. Rebase and squash into a single commit

The branch has 4 commits including a merge commit:

40333d1 Merge remote-tracking branch 'upstream/main'
c05b991 feat: private helper function: config from data
334ab5c feat: use testify instead of if/fataf, use config rather than hardcoded
ec73446 feat: improve test coverage for cmd/api 40.5 -> 81

Please rebase onto main and squash into a single commit. This keeps the git history clean. If you'd like a refresher on rebasing, I wrote a blog post that walks through it step by step: https://www.brethorsting.com/blog/2026/01/git-rebase-for-the-terrified/

Fit and Finish

4. gofmt formatting issues (all 3 files)

Please run go fmt ./cmd/api/... before committing. Specific issues:

• app.go:42: Double space in return gtfsCfg
• app_test.go:565: Trailing tab on blank line
• app_test.go:581: Extra space before comma in gtfsCfg.GTFSDataPath , parsed[...]
• main.go:68: Trailing tab on blank line

5. Hardcoded "development" assertion (app_test.go:570)

This was part of item #5 from the last review. The env assertion is still hardcoded. Since cfg.Env is an enum, you could derive the expected string the same way dumpConfigJSON does, or simply omit this assertion — the port and URL assertions already confirm the config round-tripped correctly.

Thanks again, and I look forward to merging this change.

Hey Bikram, you've made strong progress here -- the auth header redaction test, the safe type assertions, the gtfsConfigFromData helper, and the defer for stdout restoration are all good. A couple of items from the last round still need attention:

Prior Feedback Status

Blocking

1. Use require.True instead of assert.True for type assertions (app_test.go:577, 580, 583)

The two-value type assertion form from my prior review is correct, but assert.True continues the test on failure. Since the very next line accesses the asserted value (e.g., staticFeed["url"]), a failed assertion means staticFeed is nil, which causes a panic -- the exact problem we were trying to avoid. Use require.True to stop the test gracefully:

// Instead of:
staticFeed, ok := parsed["gtfs-static-feed"].(map[string]interface{})
assert.True(t, ok, "gtfs-static-feed should be a map")
assert.Equal(t, gtfsCfg.GtfsURL, staticFeed["url"])  // panics if ok is false

// Use:
staticFeed, ok := parsed["gtfs-static-feed"].(map[string]interface{})
require.True(t, ok, "gtfs-static-feed should be a map")
assert.Equal(t, gtfsCfg.GtfsURL, staticFeed["url"])  // safe: test stopped if ok was false

Same change needed for feeds (line 580) and rtFeed (line 583).

2. Rebase and squash into a single commit

This was item #3 from my last review. The branch currently has 4 commits:

fad359b feat: improve test coverage for cmd/api 40.5 -> 81
2a6001c feat: use testify instead of if/fataf, use config rather than hardcoded
0cb068a feat: private helper function: config from data
9efbb43 feat: add test for type allocation, auth header redaction, rm develop…

Please squash these into a single commit. If you'd like a refresher, see: https://www.brethorsting.com/blog/2026/01/git-rebase-for-the-terrified/

3. CI checks not running

Only the CLA check is visible -- the lint and test workflows haven't triggered. This may require a maintainer to approve the workflow run for your fork, but please verify that tests pass locally before pushing:

make test && make lint

Fit and Finish

4. Typo in assertion message (app_test.go:580)

// Before:
assert.True(t, ok, "gtfs-rt-feeds shoulde be an array of maps")

// After:
require.True(t, ok, "gtfs-rt-feeds should be an array of maps")

Strengths

• Auth header redaction test: Setting RealTimeAuthHeaderValue to a known secret, then asserting it's replaced with ***REDACTED*** is exactly the right approach. This exercises security-sensitive code that was previously untested.
• gtfsConfigFromData helper: Cleans up three call sites and prevents future drift when new fields are added to the GTFS config.
• TestRun_GracefulShutdown: The only test that exercises the Run() function directly. The errCh + time.After pattern is clean.
• Good use of defer for stdout restoration: Protects against panics in dumpConfigJSON.

Recommended Action

Fix items #1-4, squash, and push. Once CI is green, this is ready to merge.

Hey Bikram, nice work getting all the prior feedback addressed -- the squashed commit, require.True for safe type assertions, the typo fix, and clean formatting are all spot on. This is ready to merge.

Prior Feedback Status

Verification

• make test: all packages pass
• make lint: 0 issues
• gofmt -d ./cmd/api/: no formatting differences
• Single squashed commit: 0b79701 feat: improve test coverage for cmd/api 40.5 -> 81

Strengths

• gtfsConfigFromData helper: Consolidates three identical struct constructions into one function. As a bonus, the test version was previously missing StaticAuthHeaderKey, StaticAuthHeaderValue, and EnableGTFSTidy -- using the helper silently fixes that gap.
• TestRun_GracefulShutdown: Clean test of the Run() function with the errCh + time.After pattern. Lightweight and focused.
• TestDumpConfigJSON_WithExampleFile: Good coverage of JSON output formatting, config round-tripping, and auth header redaction. The defer for stdout restoration and require.True for type assertions make this robust.
• Persistence: Four rounds of review with steady, thorough improvements each time. Well done.