regression test for: migration: unexpected type of dns: map[string]interface #4848
Conversation
This validates 4a7b4d0, fix for AdguardTeam#4846
marco-m
changed the title
| // - name: "..." | ||
| // password: "..." | ||
| // - name: "..." | ||
| // password: "..." |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
We usually group asserts 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.
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