test(oracle): cover invalid Select destinations and HealthCheck ctx propagation

[Ru22-s]

Summary

Refactors edge-case tests in oracle_test.go using MockConnection to verify context propagation during HealthCheck and expand validation for non-slice destination pointers in Select.

What Changed

• Removed edgeStubConn: Replaced the custom connection stub with getOracleTestConnection() to resolve linter warnings and formatting issues.
• Added Test_Oracle_HealthCheck_PingUsesCallerContext: Verifies that c.HealthCheck passes the exact caller context down to c.conn.Ping().
• Added Test_Select_InvalidDestType_Cases: Table-driven tests verifying pointer-to-map and pointer-to-struct destinations are rejected with errInvalidDestType prior to invoking the database driver.

Testing Executed

cd pkg/gofr/datasource/oracle && go test -race -count=3 ./...

[NitinKumar004]

Hi @Ru22-s, thanks for adding more coverage to the Oracle datasource! I pulled the branch and ran it against current development. Build, vet and go test -race -count=3 all pass, and it merges cleanly. There are a couple of things CI will trip on, plus some suggestions to make the tests check what they claim.

Blocking

1. oracle_test.go:976 is not gofmt'd. The one-line Exec stub isn't aligned with the Select stub below it. Running gofmt -w (or golangci-lint fmt) on the file fixes it.
2. oracle_test.go:976-977: golangci-lint (revive unused-receiver) reports two new issues because the c receiver is unused. The submodule lint job only fails on new issues, so this will turn it red. Both 1 and 2 go away with the suggestion below, since the stub isn't needed.

Suggestions

• The stub isn't needed. The package already has a generated MockConnection (mock_interface.go), and the rest of this file uses it through getOracleTestConnection. Using it keeps things consistent and removes both lint issues.

• TestClient_HealthCheck_Timeout doesn't really exercise deadline handling in HealthCheck. HealthCheck just passes the caller's ctx to Ping and maps any error to DOWN. The ctx.Done() logic lives in the stub, so the DOWN assertion duplicates Test_Oracle_HealthDOWN. The useful thing to pin is that the caller's ctx reaches Ping. Right now, if someone changed HealthCheck to c.conn.Ping(context.Background()), this test would hang until the go test timeout instead of failing. Matching on the exact ctx with gomock makes it fail at once, with no wall-clock wait:

  func Test_Oracle_HealthCheck_PingUsesCallerContext(t *testing.T) {
  	mockConn, _, c := getOracleTestConnection(t)
  
  	ctx, cancel := context.WithCancel(t.Context())
  	cancel()
  
  	mockConn.EXPECT().Ping(ctx).Return(context.Canceled)
  
  	resp, err := c.HealthCheck(ctx)
  
  	require.ErrorIs(t, err, errStatusDown)
  
  	health, ok := resp.(*Health)
  	require.True(t, ok)
  	assert.Equal(t, StatusDown, health.Status)
  }

• TestClient_Select_InvalidDestEdgeCases.

  • The pointer-to-map and pointer-to-struct rows are a good addition. They cover the Elem().Kind() != reflect.Slice half of the check, which no existing test covers.
  • But the client is built with New(), so c.conn is nil. If that check regressed, the test would fail with a nil-pointer panic, not a readable failure.
  • The non-pointer rows are already covered by Test_Select_InvalidDestType.
  • Using the mock with .Times(0) gives a clear "unexpected call to Select" instead:

  func Test_Select_InvalidDestType_Cases(t *testing.T) {
  	tests := []struct {
  		desc string
  		dest any
  	}{
  		{desc: "pointer to map", dest: &map[string]any{}},
  		{desc: "pointer to struct", dest: &Health{}},
  	}
  
  	for _, tc := range tests {
  		t.Run(tc.desc, func(t *testing.T) {
  			mockConn, _, c := getOracleTestConnection(t)
  
  			mockConn.EXPECT().Select(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
  
  			err := c.Select(t.Context(), tc.dest, "SELECT 1 FROM dual")
  
  			require.ErrorIs(t, err, errInvalidDestType)
  		})
  	}
  }

  Alternatively, you could fold these rows into the existing Test_Select_InvalidDestType as a table.

• Naming. The file uses Test_Oracle_* / Test_Select_*, so matching that style would be nice. Also, pingErr on the stub is never set.

• Coverage. For what it's worth, package coverage is the same before and after (84.9%, with HealthCheck and Select already at 100%). The value here is in the mutations the new rows catch, not in the coverage number. It would be good to adjust the "increases coverage" and "surface early" lines in the description.

PR housekeeping

• Could you use a conventional-commit title, e.g. test(oracle): cover invalid Select destinations and HealthCheck ctx propagation?
• The test command in the description should be run from the submodule, since oracle has its own go.mod: cd pkg/gofr/datasource/oracle && go test -race ./.... The ./pkg/datasource/oracle/... path doesn't exist.
• For future PRs, please open them from a feature branch on your fork rather than its development branch, and link the issue this addresses if there is one.

Thanks again. Once the formatting and lint issues are fixed, this should be good to go.

[Ru22-s]

Hi @NitinKumar004,

Thank you for the thorough review! I've pushed an update addressing all points:

1. Removed edgeStubConn entirely and switched to getOracleTestConnection() to fix the linter warning (revive) and method alignment.
2. Added Test_Oracle_HealthCheck_PingUsesCallerContext to verify context propagation directly via gomock.
3. Updated Test_Select_InvalidDestType_Cases to test &map and &struct with mockConn.EXPECT().Select(...).Times(0).
4. Updated the PR title to follow conventional commits and corrected the test command path in the description.

Ready for re-review when you have a moment!

[NitinKumar004]

Thanks for the quick update, @Ru22-s, and for going through each point. I re-checked the latest head (173be33) locally.

What I verified as fixed

• The hand-written stub is gone, and both tests use MockConnection via getOracleTestConnection. gofmt -l is clean, and the full golangci-lint v2.12.2 run shows the same two existing govet notes as development, with no new issues.
• Test_Oracle_HealthCheck_PingUsesCallerContext now pins context propagation directly. If HealthCheck pings with context.Background(), it fails immediately with an unexpected-call error instead of hanging. It also fails if HealthCheck reports UP on a Ping error.
• Test_Select_InvalidDestType_Cases fails with a clear "unexpected call to Select" if the destination check is removed or reduced to only the pointer check. That second case is a real gap: with only Kind() != reflect.Ptr, all the existing tests still pass, so this test is what catches it.
• The title and description are updated, and the test command path is correct now.

What I ran

• go vet, and go test -race -count=3 in pkg/gofr/datasource/oracle: all pass, no flakiness.
• The net diff against development is just the two tests in oracle_test.go, and it merges cleanly.

No remaining concerns from my side. One small thing for next time: opening PRs from a feature branch on your fork, rather than its development branch, makes it easier to keep your fork in sync. This one looks good to merge. CI hasn't run yet because the workflows are waiting for maintainer approval.