Skip metrics/traces with refactor #105
Conversation
internal/transform/suites.go
Outdated
| Meter(string) metric.Meter | ||
| } | ||
|
|
||
| func TransformAndLoadSuites(ctx context.Context, cfg *config.Config, provider OtelProvider, |
There was a problem hiding this comment.
nit: is the function name duplicating the package name? Having to use it like this transform.TransformAndLoadSuites seems weird. The natural AndLoadSuites sounds incorrect, although not sure about WithLoadSuites. wdyt?
There was a problem hiding this comment.
I will think about this on my walk and adjust the name when I get back.
|
|
||
| import "os" | ||
|
|
||
| type FileReader struct { |
There was a problem hiding this comment.
question: if this is used only in tests, do we need it to be exposed, even under the internal package?
There was a problem hiding this comment.
In the future we may use it for more than tests. If a switch is added to take a file path we can use it for that.
I agree. I almost made a separate PR. I wanted to also have the skip metric/trace tests to demonstrate why the |
|
At some point I'm going to want to add an interface mocker to improve test coverage. Any objections or preferences on which? |
ryanrolds
force-pushed
the
rolds/refactor_for_better_testing
branch
from
11567df to
723d5c6
Compare
|
I made some adjustments that I think addresses the concerns with the naming. |
|
@ryanrolds I've not forgotten this 🙏 I'm at Gophercon SG returning back to Spain next Monday. |
Here is a purposed implementation of skip metrics/traces with a refactoring of the project structure and introduction of a config package. The refactor made adding the tests easier and will make implementing tests easier in the future.
Please let me know what you think. I'm happy to adjust it.
Major changes
internal.intenal/configpackage to handle configurationMaintoRunTestReadertoFileReaderFakeGitRepothat required the samples existing in the same dir as the test and the root of the repo