Skip to content

confgenerator: use golden package in unit tests #811

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
Merged
merged 14 commits into from
Sep 8, 2022
Merged

confgenerator: use golden package in unit tests #811

merged 14 commits into from
Sep 8, 2022

Conversation

@braydonk
Copy link
Contributor

@braydonk braydonk commented Aug 24, 2022 •
edited
Loading

Description

The gotesttools package for golden tests lines up perfectly with the
golden tests that we currently do, and using it can greatly simplify our
unit test code. This PR reimagines the confgenerator_test.go tests using
this package. A few other minor changes were made:

  • The invalid type for sqlserver test did not have an input.yaml (this
    new way of doing tests actually caught that; it's been that way
    forever)
  • It never made much sense for golden_otel to have an extension of
    conf, this PR switches that to yaml
  • Since we install gotesttools in this PR for golden, just opted to use the assert package as well

Related issue

None yet, since I wanted to post this as RFC first. 🙂
EDIT: Created a bug for this. b/243791505

How has this been tested?

Verbose test run: https://pastebin.com/SahVB1Kg
The -update flag worked as expected, as evidenced by the updated files in the PR.

Checklist:

  • Unit tests
    • Unit tests do not apply.
    • Unit tests have been added/modified and passed for this PR.
  • Integration tests
    • Integration tests do not apply.
    • Integration tests have been added/modified and passed for this PR.
  • Documentation
    • This PR introduces no user visible changes.
    • This PR introduces user visible changes and the corresponding documentation change has been made.
  • Minor version bump
    • This PR introduces no new features.
    • This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
    • This PR bumps the version.

@sophieyfang sophieyfang self-requested a review August 25, 2022 13:45
@qingling128
Copy link
Contributor

qingling128 commented Aug 25, 2022 •
edited
Loading

Can we add some details (e.g. command output, transcripts, could be a file) for the How has this been tested? section for the records? BTW, have we gotten a chance to test the -update_golden flag with this change? (no need to wait for me for merging)

@braydonk braydonk force-pushed the braydonk-golden-experiment branch from 31189b2 to e0fbaed Compare September 6, 2022 15:41
@braydonk
confgenerator: use golden package in unit tests
c97a39d 
The gotesttools package for golden tests lines up perfectly with the
golden tests that we currently do, and using it can greatly simplify our
unit test code. This PR reimagines the confgenerator_test.go tests using
this package. 2 other minor changes were made:

* The invalid type for sqlserver test did not have an input.yaml (this
  new way of doing tests actually caught that; it's been that way
  forever)
* It never made much sense for `golden_otel` to have an extension of
  `conf`, this PR switches that to `yaml`
@braydonk
ci: adding crlf normalization env var
e4e378a
@braydonk
try turning off autocrlf
0a1793d
@braydonk
I had the env var in the wrong spot before
32b5d18
@braydonk
Revert "I had the env var in the wrong spot before"
32bbc33 
This reverts commit 13b99e9f40387531f6cde67163f7ccaffbceebc3. It did not
work.
@braydonk
Makefile: fix update_golden target
aee497e
@braydonk
confgenerator: refactor checkGoldenError function
3dc1f36 
The local function checkGoldenError was unnecessarily complex compared
to simply splitting it up and calling assert in every spot. Thanks
@avilevy18 for the suggestion.
@braydonk
confgenerator: create builtin test case
ea927dd
@braydonk
confgenerator: simplify test logic, test lua files
fb283fb 
Unifies the tests for valid and invalid to reduce repetition. Also
supports generating and comparing lua files, as well as cleaning up old
ones when the lua files change.
@braydonk
Rebased and regenerated configs
2ce3415
@braydonk
revert and regenerate configs
1fb59a6 
I had a bug in the lua gen testing that botched tons of input.yaml
files. I reverted that commit, restored the new confgenerator_test
changes, and regenerated the configs. I should be smarter about when I
commit!
@braydonk
confgenerator: reduce code repetition
8eab128 
By using a functional option to runTestsInDir, we can reduce the
duplication of gathering all the platform's test cases and asserting
differently for valid vs invalid tests.
@braydonk
resolve leftover conflicts
b0d3e14
avilevy18
avilevy18 reviewed Sep 6, 2022
View reviewed changes
@braydonk braydonk force-pushed the braydonk-golden-experiment branch from 329db6f to e3d7607 Compare September 6, 2022 19:04
@braydonk braydonk requested a review from sophieyfang September 8, 2022 11:29
@braydonk braydonk merged commit d12b900 into master Sep 8, 2022
@quentinmit
Copy link
Member

quentinmit commented Sep 26, 2022

I'm trying to handle a merge conflict on this PR - I'm wondering why this refactoring added so much duplicate code? The golden package is nice but it seems like it was coupled with unrolling a bunch of loops?

@braydonk
Copy link
Contributor Author

braydonk commented Sep 28, 2022

@quentinmit The unrolling of loops was to add some robustness to the tests.
In the previous version, valid and invalid tests weren't technically treated any differently. If an invalid config was placed in the valid folder or vice versa the previous tests didn't care. There were some actual test cases before this PR that were afflicted by this. Now they are properly treated differently, i.e. if an invalid config is put in a valid test case folder or vice versa the test will fail.
When Lua files changed in the previous version there was no cleanup. Now, the case where they are changed includes cleanup of the old Lua files.
The previous version of the tests checked for a test name that contained "builtin" which I thought would make more sense to split into its own test run rather than needing to check and do something different on that case. A lot of folks also use that builtin test case a lot to check what the default config generation is, so separating it into its own folder makes it more convenient to find (pretty minor thing compared to everything else).

@igorpeshansky igorpeshansky deleted the braydonk-golden-experiment branch July 10, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avilevy18 avilevy18 avilevy18 left review comments

@sophieyfang sophieyfang sophieyfang 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.

6 participants

@braydonk @qingling128 @quentinmit @sophieyfang @avilevy18