Move golden files below testdata/golden #232
Conversation
adombeck
force-pushed
the
move-golden-files
branch
from
1960249
to
90d0a80
Compare
There was a problem hiding this comment.
Regarding the changes, I have some comments but nothing big.
However, I do have 2 observations:
-
Did you try reupdating the golden files to check if everything is ok? (I'm assuming you did, but it doesn't hurt to ask).
-
We need to maintain consistency between our projects. If we agree on this new structure, this also implies applying these changes to
authd,adsys, WSL and so on.
internal/broker/broker_test.go
Outdated
| @@ -12,6 +12,7 @@ import ( | |||
| "github.com/stretchr/testify/require" | |||
| "github.com/ubuntu/authd-oidc-brokers/internal/broker" | |||
| "github.com/ubuntu/authd-oidc-brokers/internal/broker/authmodes" | |||
| "github.com/ubuntu/authd-oidc-brokers/internal/golden" | |||
There was a problem hiding this comment.
I can see the appeal here, but can you make it a subpackage of testutils? I don't think this belongs solely on internal/. This way, we can still relate to it as golden.CheckOrUpdate, but at least it's organized better.
internal/golden/golden.go
Outdated
| // GoldenOption is a supported option reference to change the golden files comparison. | ||
| type GoldenOption func(*goldenOptions) | ||
| // Option is a supported option reference to change the golden files comparison. | ||
| type Option func(*goldenOptions) |
There was a problem hiding this comment.
Probably rename the goldenOptions type to options also?
There was a problem hiding this comment.
I renamed it to config to avoid name collisions
internal/golden/golden.go
Outdated
| err = f.Close() | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
👀 (you already know my comment...)
adombeck
force-pushed
the
move-golden-files
branch
2 times, most recently
from
4539434
to
93dd088
Compare
We used to store golden files in:
testdata/TopLevelTestName/golden/SubTestName
We now store them in:
testdata/golden/TopLevelTestName/SubTestName
That allows to easily copy/paste the full test name (top-level test name
+ subtest name) into a `go test --run` command.
The package name is already "golden", so we don't have to mention it again in the exported names.
Avoid duplicating the package name in the struct name but also avoid using the name "options" again which is already used as a parameter name and local variable name in this package.
adombeck
force-pushed
the
move-golden-files
branch
from
93dd088
to
85bf882
Compare
Yes, all the golden files are updated.
Right. My proposal is to change the structure for the reason mentioned above. If you or @didrocks (should I tag anyone else?) see any problems with that, I can split off the other changes into a separate PR. If I understood @didrocks correctly during the standup, he also prefers this directory structure. |
Yeah, I think that ought to be 2 PRs, as we discussed in the HO, but that’s ok now. Just ensure there is a Jira card created on moving most of |
Tracked in UDENG-5363 |
We used to store golden files in:
We now store them in:
That allows to easily copy/paste the full test name (top-level test name + subtest name) into a
go test --runcommand.