-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
regression test for: migration: unexpected type of dns: map[string]interface #4848
base: master
Are you sure you want to change the base?
Conversation
This validates 4a7b4d0, fix for AdguardTeam#4846
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! We have some comments regarding formatting and comments.
// - name: "..." | ||
// password: "..." | ||
// - name: "..." | ||
// password: "..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, these format changes have been made by the Go 1.19 comment formatter as opposed to manual reformatting, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As sorry I missed this :-(
Yes, it is the Go 1.10 comment formatter :-/.
I can remove this if you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you don't need to, I'm just making sure.
@@ -720,7 +719,7 @@ func upgradeSchema12to13(diskConf yobj) (err error) { | |||
var dns yobj | |||
dns, ok = dnsVal.(yobj) | |||
if !ok { | |||
return fmt.Errorf("unexpected type of dns: %T", dnsVal) | |||
return fmt.Errorf("unexpected type of dns: %T; want: %T", dnsVal, dns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few other similar errors throughout the recent migrations, and it would be nice to have them all in a unified format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added this to help me understand better what the code expected, but I agree it is more a debug printf.
I am not sure I understand your comment thou: do you want me to revert this change or to extend it to all the similar "unexpected ..." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that it's better to replace all instances of errors like unexpected type of %s: %T
with this new form that includes the expected type as opposed to just here.
@@ -558,6 +560,18 @@ func TestUpgradeSchema12to13(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestUpgradeSchema12to13Regression(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like a better regression test would try to migrate a configuration file going all the way back to version 1
. Please add a TODO assigned to me about that.
@@ -558,6 +560,18 @@ func TestUpgradeSchema12to13(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestUpgradeSchema12to13Regression(t *testing.T) { | |||
body, err := os.ReadFile("testdata/12to13-regression.yaml") | |||
assert.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should be a require
, and also split from the rest by an empty line.
|
||
err = upgradeSchema12to13(diskConf) | ||
|
||
assert.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually group assert
s and requires
together with the statements where the data was acquired, so please remove the empty line above.
@@ -0,0 +1,3 @@ | |||
schema_version: 12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, too, a TODO should be added to make it a more faithful representation of a config.yaml
at version 12
. With YAML, you can never be too defensive, heh.
I stumbled upon the same problem of #4846:
I was not sure how to fix, but after having added a regression test, I saw the fix 4a7b4d0 by @ainar-g.
The interest of this PR is in the regression test, that AdGuardHome currently doesn't have.
If you disagree, feel free to close.
Thanks for AdGuardHome!
This validates 4a7b4d0, fix for #4846