-
Notifications
You must be signed in to change notification settings - Fork 3
new test for params.go #139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive unit tests for the params.go file and the mask utility function, addressing issue #138. The tests validate the correct initialization of mock parameters and ensure the token masking functionality works as expected.
Key changes:
- Added 16 tests for the
NewMockParams()function covering most configuration fields - Added 4 tests for the
mask()utility function covering basic masking scenarios
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/client/params_test.go
Outdated
| func TestMask_ReturnsEmptyForEmptyToken(t *testing.T) { | ||
| result := mask("") | ||
| assert.Equal(t, "", result, "mask of empty string must be empty") | ||
| } | ||
|
|
||
| func TestMask_MasksShortToken(t *testing.T) { | ||
| result := mask("AB") | ||
| assert.Equal(t, "AB", result, "short token must show visible part") | ||
| } | ||
|
|
||
| func TestMask_MasksLongToken(t *testing.T) { | ||
| result := mask("ABCDEFGH") | ||
| assert.Equal(t, "ABC*****", result, "long token must be masked") | ||
| } | ||
|
|
||
| func TestMask_ShowsThreeCharacters(t *testing.T) { | ||
| result := mask("secrettoken12345") | ||
| assert.NotEmpty(t, result, "masked token must not be empty") | ||
| assert.Equal(t, "sec*************", result, "token must show first three characters") | ||
| } |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mask function being tested is located in refrax_client.go, not params.go. Consider moving these tests to refrax_client_test.go or renaming this test file to better reflect that it tests multiple components from the client package, not just params.go.
| func TestNewMockParams_CreatesValidInstance(t *testing.T) { | ||
| params := NewMockParams() | ||
| require.NotNil(t, params, "params must not be nil") | ||
| } | ||
|
|
||
| func TestNewMockParams_SetsMockProvider(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.Equal(t, "mock", params.Provider, "provider must be mock") | ||
| } | ||
|
|
||
| func TestNewMockParams_SetsDefaultToken(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.Equal(t, "ABC", params.Token, "token must be ABC") | ||
| } | ||
|
|
||
| func TestNewMockParams_EnablesMockProject(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.True(t, params.MockProject, "mock project must be enabled") | ||
| } | ||
|
|
||
| func TestNewMockParams_DisablesDebug(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.False(t, params.Debug, "debug must be disabled") | ||
| } | ||
|
|
||
| func TestNewMockParams_DisablesStats(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.False(t, params.Stats, "stats must be disabled") | ||
| } | ||
|
|
||
| func TestNewMockParams_SetsStdFormat(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.Equal(t, "std", params.Format, "format must be std") | ||
| } | ||
|
|
||
| func TestNewMockParams_SetsDefaultMaxSize(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.Equal(t, 200, params.MaxSize, "max size must be 200") | ||
| } | ||
|
|
||
| func TestNewMockParams_SetsDiscardLog(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.Equal(t, io.Discard, params.Log, "log must be io.Discard") | ||
| } | ||
|
|
||
| func TestNewMockParams_SetsDefaultChecks(t *testing.T) { | ||
| params := NewMockParams() | ||
| require.Equal(t, 1, len(params.Checks), "must have one check") | ||
| assert.Equal(t, "mvn clean test", params.Checks[0], "check must be mvn clean test") | ||
| } | ||
|
|
||
| func TestNewMockParams_DisablesColorless(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.False(t, params.Colorless, "colorless must be disabled") | ||
| } | ||
|
|
||
| func TestNewMockParams_SetsDefaultModel(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.Equal(t, "gpt-3.5-turbo", params.Model, "model must be gpt-3.5-turbo") | ||
| } | ||
|
|
||
| func TestNewMockParams_SetsDefaultAttempts(t *testing.T) { | ||
| params := NewMockParams() | ||
| assert.Equal(t, 3, params.Attempts, "attempts must be 3") | ||
| } |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests for NewMockParams do not cover all fields set by the function. Missing test coverage for: Playbook (should be empty string), Soutput (should be "stats"), Input (should be empty string), and Output (should be empty string). Consider adding tests for these fields to ensure complete coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
internal/client/params_test.go
Outdated
| func TestMask_ReturnsEmptyForEmptyToken(t *testing.T) { | ||
| result := mask("") | ||
| assert.Equal(t, "", result, "mask of empty string must be empty") | ||
| } | ||
|
|
||
| func TestMask_MasksShortToken(t *testing.T) { | ||
| result := mask("AB") | ||
| assert.Equal(t, "AB", result, "short token must show visible part") | ||
| } | ||
|
|
||
| func TestMask_MasksLongToken(t *testing.T) { | ||
| result := mask("ABCDEFGH") | ||
| assert.Equal(t, "ABC*****", result, "long token must be masked") | ||
| } | ||
|
|
||
| func TestMask_ShowsThreeCharacters(t *testing.T) { | ||
| result := mask("secrettoken12345") | ||
| assert.NotEmpty(t, result, "masked token must not be empty") | ||
| assert.Equal(t, "sec*************", result, "token must show first three characters") | ||
| } |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mask function tests are missing important edge cases. Consider adding tests for: a single-character token (length 1), a three-character token (length 3, the boundary where visible equals token length), and a four-character token (length 4, the first case where masking occurs with one asterisk).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <[email protected]>
Co-authored-by: yegor256 <[email protected]>
Co-authored-by: yegor256 <[email protected]>
Add missing test coverage for NewMockParams fields
Add edge case tests for mask function boundary conditions
internal/client/params_test.go
Outdated
| assert.Equal(t, "", params.Output, "output must be empty") | ||
| } | ||
|
|
||
| func TestMask_ReturnsEmptyForEmptyToken(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 All the TestMask are irrelevant to the params.go. Moreover, the mask function (refrax_client.go) is a private one, thus there is no need to test it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix the problem just mentioned
internal/client/params_test.go
Outdated
| } | ||
|
|
||
| func TestNewMockParams_DisablesStats(t *testing.T) { | ||
| params := NewMockParams() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 Redundant variable?
https://www.yegor256.com/2015/09/01/redundant-variables-are-evil.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix this problem, get rid of the redundant variable
Co-authored-by: yegor256 <[email protected]>
Co-authored-by: yegor256 <[email protected]>
Remove private function tests from params_test.go
Remove redundant variable in TestNewMockParams_DisablesStats
|
@volodya-lombrozo fixed, please review again |
related to #138