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

Decode / encode tests #4443

Merged
merged 12 commits into from
Jul 26, 2023
Merged

Decode / encode tests #4443

merged 12 commits into from
Jul 26, 2023

Conversation

thampiotr
Copy link
Contributor

@thampiotr thampiotr commented Jul 13, 2023 •
edited
Loading

PR Description

This PR adds tests that cover different cases of nested blocks with defaults etc.

  • Encodes given block to river
  • Verifies river matches the expectations
  • Decodes river back to a struct
  • Compares the result with original

Furthermore, this PR:

  • Fixes one small issue in encoding logic
  • Adds docs with details how Defaulter works
  • Refactors a couple of Default-related fragments of code (after reviewing all the Defaulters to make sure they agree on defaults between outer and inner blocks)

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@thampiotr thampiotr force-pushed the thampiotr/decode-encode-tests branch from 3c9cc98 to aa98c70 Compare July 14, 2023 10:21
thampiotr
thampiotr commented Jul 14, 2023
View reviewed changes
pkg/river/token/builder/builder.go
Comment on lines -219 to -225
case fieldValue.IsZero():
// It shouldn't be possible to have a required block which is unset, but
// we'll encode something anyway.
inner := NewBlock(fullName, "")
inner.body.SetValueOverrideHook(b.valueOverrideHook)
b.AppendBlock(inner)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here a zero struct is encoded as empty block in River. When it's decoded, the defaults will be applied, so we wouldn't get the same value as we encoded.

Luckily all we need to do is remove this case and the other cases will handle it correctly, taking defaults into account.

@thampiotr thampiotr force-pushed the thampiotr/decode-encode-tests branch from ab0fc11 to 2539217 Compare July 14, 2023 12:54
@thampiotr thampiotr mentioned this pull request Jul 14, 2023
Merged
3 tasks
@thampiotr thampiotr marked this pull request as ready for review July 14, 2023 13:16
@thampiotr thampiotr requested review from a team as code owners July 14, 2023 13:16
rfratto
rfratto reviewed Jul 14, 2023
View reviewed changes
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Thanks Piotr!

pkg/river/internal/value/decode.go Outdated Show resolved Hide resolved
pkg/river/token/builder/blocks_nesting_test.go Outdated Show resolved Hide resolved
@thampiotr thampiotr requested a review from rfratto July 19, 2023 17:53
rfratto
rfratto approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM. I'm considering whether we should merge this as-is and deal with the commented out tests in a follow up discussion. WDYT?

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/river/token/builder/nested_defaults_test.go Outdated Show resolved Hide resolved
@thampiotr thampiotr force-pushed the thampiotr/decode-encode-tests branch from 51c2091 to 30ecfc3 Compare July 24, 2023 10:45
@tpaschalis tpaschalis merged commit c74a4f8 into main Jul 26, 2023
6 checks passed
@tpaschalis tpaschalis deleted the thampiotr/decode-encode-tests branch July 26, 2023 12:46
dwalker-sabiogroup pushed a commit to dwalker-sabiogroup/agent that referenced this pull request Jul 28, 2023
@thampiotr @dwalker-sabiogroup
river: add encode/decode tests (grafana#4443)
5eca851
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
@thampiotr @clayton-cornell
river: add encode/decode tests (#4443)
2f1ea0d
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
@thampiotr @clayton-cornell
river: add encode/decode tests (#4443)
474ef9b
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rfratto rfratto rfratto approved these changes

@erikbaranowski erikbaranowski Awaiting requested review from erikbaranowski

Assignees
No one assigned
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thampiotr @rfratto @tpaschalis