tests: update unit tests #68
Conversation
Additionally, I'm instrumenting the integration test, and merging the coverage profile so that we have a combined measure of the code exercised by coverage and unit tests.
| // WithConfigFile configures OpenVPNOptions parsed from the given file. | ||
| func WithConfigFile(configPath string) Option { | ||
| return func(config *Config) { | ||
| openvpnOpts, err := ReadConfigFile(configPath) |
There was a problem hiding this comment.
I am not super convinced by panicking here. Ideally, given that this is a library function, I suppose it would instead be better to return an error to the caller. A strategy to do this could be the following:
-
you redefine
Optionasfunc(*Config) error -
when you are invoking the sequence of functions you in
NewConfig, you also check for errors
There was a problem hiding this comment.
true, I agree that panic is not a good thing to do here. I got too used to add a few asserts for incorrect values, but this is public-facing API.
I think I would prefer to move this to a new issue to work after merging #70 , since I've split and moved the code dealing with config (created #69 to track this).
| } | ||
|
|
||
| func parseProto(p []string, o *OpenVPNOptions) error { | ||
| func parseProto(p []string, o *OpenVPNOptions) (*OpenVPNOptions, error) { |
There was a problem hiding this comment.
I'm missing some information to fully understand this change in pattern. AFAICT, it should not be necessary to return the o pointer, given that you're mutating it and it's available to the caller--can you elaborate?
There was a problem hiding this comment.
I suspect my reasoning here was to move to return a copy in the future and not modify the passed struct, but you're right that this might be unnecessary, especially since there's no other modification that uses this value.
do you think it's better to revert to the previous state and just return err?
There was a problem hiding this comment.
fwiw, I think mutating makes for confusing tests
617 name: "default",
618 args: args{o: &OpenVPNOptions{}},
619 wantErr: nil,
and then instead of got := foo(tt.args.o} I need to check for mutation of the argument. I think it makes a cleaner API, but this needs more work than done here.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer mustClose(f) |
| // Package optional implements optional values. | ||
| // Package optional contains safer code to handle optional values. | ||
| // This package is taken from probe-cli/internal/optional. | ||
| // Copyright 2024, Simone Basso |
There was a problem hiding this comment.
| // Copyright 2024, Simone Basso |
I think there's no need, but thank you 😊
There was a problem hiding this comment.
I think it's a good practice when copying code verbatim. software archaeologists from the future will be grateful.
|
|
||
| // Copyright 2024, Simone Basso |
There was a problem hiding this comment.
| // Copyright 2024, Simone Basso |
aaaw, there's no need! 😊
Co-authored-by: Simone Basso <bassosimone@gmail.com>
Co-authored-by: Simone Basso <bassosimone@gmail.com>
Co-authored-by: Simone Basso <bassosimone@gmail.com>
Co-authored-by: Simone Basso <bassosimone@gmail.com>
Additionally, I'm instrumenting the integration test, and merging the coverage profile so that we have a combined measure of the code exercised by coverage and unit tests.
Checklist
ARCHITECTURE.md.