-
Notifications
You must be signed in to change notification settings - Fork 39
Chore/improve unit test cov pkl pkg #224
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
e4ab3b2
5296454
d8c9571
d474caf
2f71a98
6aafc62
cae0762
a22dd59
403f8cc
0fbd845
95e479e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // Copyright © 2026 Apple Inc. and the Pkl project authors. All rights reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // https://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| package pkl | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestEvalError_Error(t *testing.T) { | ||
| tests := map[string]struct { | ||
| err EvalError | ||
| want string | ||
| }{ | ||
| "should return error output message": { | ||
| err: EvalError{ErrorOutput: "some eval error"}, | ||
| want: "some eval error", | ||
| }, | ||
| "should return empty string when error output is empty": { | ||
| err: EvalError{}, | ||
| want: "", | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() | ||
| assert.Equal(t, tc.want, tc.err.Error()) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestEvalError_Is(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have similar concerns here about test value since this is almost entirely testing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree, should be addressed by this comment |
||
| tests := map[string]struct { | ||
| target error | ||
| want bool | ||
| }{ | ||
| "should return true when target is an EvalError": { | ||
| target: &EvalError{ErrorOutput: "some error"}, | ||
| want: true, | ||
| }, | ||
| "should return true when target is a different EvalError": { | ||
| target: &EvalError{ErrorOutput: "different error"}, | ||
| want: true, | ||
| }, | ||
| "should return false when target is nil": { | ||
| target: nil, | ||
| want: false, | ||
| }, | ||
| "should return false when target is a different error type": { | ||
| target: fmt.Errorf("some other error"), | ||
| want: false, | ||
| }, | ||
| "should return false when target is an InternalError": { | ||
| target: &InternalError{err: fmt.Errorf("internal")}, | ||
| want: false, | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() | ||
| evalErr := &EvalError{ErrorOutput: "eval error"} | ||
| assert.Equal(t, tc.want, evalErr.Is(tc.target)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestInternalError_Error(t *testing.T) { | ||
| tests := map[string]struct { | ||
| err InternalError | ||
| want string | ||
| }{ | ||
| "should return formatted internal error message": { | ||
| err: InternalError{err: fmt.Errorf("something broke")}, | ||
| want: "an internal error occurred: something broke", | ||
| }, | ||
| "should return formatted message with nil inner error": { | ||
| err: InternalError{err: nil}, | ||
| want: "an internal error occurred: <nil>", | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() | ||
| assert.Equal(t, tc.want, tc.err.Error()) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestInternalError_Is(t *testing.T) { | ||
| tests := map[string]struct { | ||
| target error | ||
| want bool | ||
| }{ | ||
| "should return true when target is an InternalError": { | ||
| target: &InternalError{err: fmt.Errorf("some error")}, | ||
| want: true, | ||
| }, | ||
| "should return true when target is a different InternalError": { | ||
| target: &InternalError{err: fmt.Errorf("different error")}, | ||
| want: true, | ||
| }, | ||
| "should return false when target is nil": { | ||
| target: nil, | ||
| want: false, | ||
| }, | ||
| "should return false when target is a different error type": { | ||
| target: fmt.Errorf("some other error"), | ||
| want: false, | ||
| }, | ||
| "should return false when target is an EvalError": { | ||
| target: &EvalError{ErrorOutput: "eval"}, | ||
| want: false, | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() | ||
| internalErr := &InternalError{err: fmt.Errorf("internal error")} | ||
| assert.Equal(t, tc.want, internalErr.Is(tc.target)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestInternalError_Unwrap(t *testing.T) { | ||
| tests := map[string]struct { | ||
| inner error | ||
| want error | ||
| }{ | ||
| "should return the wrapped error": { | ||
| inner: fmt.Errorf("wrapped error"), | ||
| want: fmt.Errorf("wrapped error"), | ||
| }, | ||
| "should return nil when inner error is nil": { | ||
| inner: nil, | ||
| want: nil, | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() | ||
| internalErr := &InternalError{err: tc.inner} | ||
| assert.Equal(t, tc.want, internalErr.Unwrap()) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestEvalError_WorksWithErrorsIs(t *testing.T) { | ||
| t.Parallel() | ||
| evalErr := &EvalError{ErrorOutput: "eval error"} | ||
| wrappedErr := fmt.Errorf("wrapped: %w", evalErr) | ||
| assert.True(t, errors.Is(wrappedErr, &EvalError{})) | ||
| } | ||
|
|
||
| func TestInternalError_WorksWithErrorsIs(t *testing.T) { | ||
| t.Parallel() | ||
| internalErr := &InternalError{err: fmt.Errorf("something broke")} | ||
| wrappedErr := fmt.Errorf("wrapped: %w", internalErr) | ||
| assert.True(t, errors.Is(wrappedErr, &InternalError{})) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,21 +53,21 @@ type ExternalReaderClientOptions struct { | |
| } | ||
|
|
||
| // WithExternalClientResourceReader adds an additional [ResourceReader] to the ExternalReaderClient. | ||
| var WithExternalClientResourceReader = func(reader ResourceReader) func(*ExternalReaderClientOptions) { | ||
| func WithExternalClientResourceReader(reader ResourceReader) func(*ExternalReaderClientOptions) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to accept non-test changes in a separate PR. A similar pattern is in use in evaluator_options.go, so it might be nice to change those at the same time as these.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @HT154, Agree, I will note them down, and I will open another PR for them |
||
| return func(options *ExternalReaderClientOptions) { | ||
| options.ResourceReaders = append(options.ResourceReaders, reader) | ||
| } | ||
| } | ||
|
|
||
| // WithExternalClientModuleReader adds an additional [ModuleReader] to the ExternalReaderClient. | ||
| var WithExternalClientModuleReader = func(reader ModuleReader) func(*ExternalReaderClientOptions) { | ||
| func WithExternalClientModuleReader(reader ModuleReader) func(*ExternalReaderClientOptions) { | ||
| return func(options *ExternalReaderClientOptions) { | ||
| options.ModuleReaders = append(options.ModuleReaders, reader) | ||
| } | ||
| } | ||
|
|
||
| // WithExternalClientStreams sets the input and output interfaces the ExternalReaderClient will use to communicate with the Pkl evaluator. | ||
| var WithExternalClientStreams = func(requestReader io.Reader, responseWriter io.Writer) func(*ExternalReaderClientOptions) { | ||
| func WithExternalClientStreams(requestReader io.Reader, responseWriter io.Writer) func(*ExternalReaderClientOptions) { | ||
| return func(options *ExternalReaderClientOptions) { | ||
| options.RequestReader = requestReader | ||
| options.ResponseWriter = responseWriter | ||
|
|
@@ -78,7 +78,7 @@ var WithExternalClientStreams = func(requestReader io.Reader, responseWriter io. | |
| // that associates the provided scheme with files from fs. | ||
| // | ||
| //goland:noinspection GoUnusedGlobalVariable | ||
| var WithExternalClientFs = func(fs fs.FS, scheme string) func(opts *ExternalReaderClientOptions) { | ||
| func WithExternalClientFs(fs fs.FS, scheme string) func(opts *ExternalReaderClientOptions) { | ||
| return func(opts *ExternalReaderClientOptions) { | ||
| reader := &fsReader{fs: fs, scheme: scheme} | ||
| WithExternalClientModuleReader(&fsModuleReader{reader})(opts) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // Copyright © 2026 Apple Inc. and the Pkl project authors. All rights reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // https://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| package pkl | ||
|
|
||
| import ( | ||
| "io" | ||
| "testing" | ||
| ) | ||
|
|
||
| type writerMock struct { | ||
| err error | ||
| bytes int | ||
| } | ||
|
|
||
| func (w writerMock) Write(_ []byte) (n int, err error) { | ||
| return w.bytes, w.err | ||
| } | ||
|
|
||
| func TestLogger(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also not a valuable test: it makes no assertions.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point above. I will totally remove it from this PR |
||
|
|
||
| tests := map[string]struct { | ||
| writerMock io.Writer | ||
| msg string | ||
| frameURI string | ||
| }{ | ||
| "should successfully log a message as trace and warn": { | ||
| writerMock: writerMock{err: nil, bytes: 20}, | ||
| msg: "test message", | ||
| frameURI: "test", | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() | ||
| lgr := NewLogger(tc.writerMock) | ||
| lgr.Trace(tc.msg, tc.frameURI) | ||
| lgr.Warn(tc.msg, tc.frameURI) | ||
| }) | ||
| } | ||
| } | ||
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.
I'm not convinced of the utility of this test case. pkl-go's error messages are not documented API and this is essentially testing the identity of the message itself. I'm not a believer in coverage for coverage's sake: what was a one-line function now comes with 22 lines of test code that must be maintained to enforce behavior that we make no guarantees about.
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.
Makes sense — the error message formatting isn't a documented API guarantee, so these tests add maintenance burden without real value. I'll remove them; the error behavior can be validated as part of e2e tests instead.