Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move golden files below testdata/golden #232

Merged
merged 9 commits into from
Dec 4, 2024
Merged

Move golden files below testdata/golden #232

merged 9 commits into from
Dec 4, 2024

Conversation

adombeck
Copy link
Contributor

@adombeck adombeck commented Nov 19, 2024 •
edited
Loading

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.

UDENG-5448

@adombeck adombeck marked this pull request as ready for review November 19, 2024 10:55
@adombeck adombeck requested a review from a team as a code owner November 19, 2024 10:55
denisonbarbosa
denisonbarbosa reviewed Nov 19, 2024
View reviewed changes
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the changes, I have some comments but nothing big.

However, I do have 2 observations:

  1. 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).

  2. 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 Show resolved Hide resolved
internal/golden/golden.go Outdated Show resolved Hide resolved
internal/golden/golden.go Outdated Show resolved Hide resolved
@adombeck adombeck force-pushed the move-golden-files branch 2 times, most recently from 4539434 to 93dd088 Compare November 19, 2024 13:32
@adombeck
Move golden files below testdata/golden
62ba9bb 
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.
@adombeck
refactor: Extract package "golden"
c48dc9c
@adombeck
refactor: Use variadic options argument
c9d4120
@adombeck
Support specifying relative path to golden file
78cc719
@adombeck
refactor: Remove "Golden" from exported names in golden package
4856781 
The package name is already "golden", so we don't have to mention it
again in the exported names.
@adombeck
Fix unhandled error
16e45fa
@adombeck
Remove import alias
d889a4f
@adombeck
golden: Add t.Helper() to helper functions
6992fed
@adombeck
refactor: Rename goldenOptions to config
85bf882 
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
Copy link
Contributor Author

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).

Yes, all the golden files are updated.

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.

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.

@didrocks
Copy link
Member

Right. My proposal is to change the structure for the reason mentioned #232 (comment). 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 testutils/ (including this subpackage) to its own repo and list all the projects to move to it. That will make ofc backport harder in the futures when we can’t land the whole content, but that’s ok for now.

@adombeck
Copy link
Contributor Author

adombeck commented Nov 20, 2024 •
edited
Loading

Just ensure there is a Jira card created on moving most of testutils/ (including this subpackage) to its own repo and list all the projects to move to it.

Tracked in UDENG-5363

jibel
jibel requested changes Dec 4, 2024
View reviewed changes
Copy link

@jibel jibel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testing gh/jira reviewers sync.

jibel
jibel approved these changes Dec 4, 2024
View reviewed changes
Copy link

@jibel jibel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another test

@adombeck adombeck merged commit 389b426 into main Dec 4, 2024
4 checks passed
@adombeck adombeck deleted the move-golden-files branch December 4, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denisonbarbosa denisonbarbosa denisonbarbosa approved these changes

@jibel jibel jibel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adombeck @didrocks @jibel @denisonbarbosa