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.

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

Make unmarshaling json config strict #279

Merged
merged 5 commits into from
Aug 3, 2022

Conversation

ronensc
Copy link
Collaborator

@ronensc ronensc commented Aug 3, 2022

Unmarshaling non-config related json still use the non-strict version.

This is a follow-up on #271
Fix #274

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2022

Codecov Report

Merging #279 (be64c8d) into main (d7656a2) will increase coverage by 0.03%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main     #279      +/-   ##
==========================================
+ Coverage   66.85%   66.89%   +0.03%     
==========================================
  Files          73       73              
  Lines        4152     4157       +5     
==========================================
+ Hits         2776     2781       +5     
  Misses       1197     1197              
  Partials      179      179              
Flag Coverage Δ
unittests 66.89% <92.30%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/confgen/confgen.go 49.20% <50.00%> (ø)
pkg/confgen/encode.go 50.00% <100.00%> (ø)
pkg/confgen/extract.go 50.00% <100.00%> (ø)
pkg/confgen/transform.go 46.66% <100.00%> (ø)
pkg/confgen/visualization.go 50.00% <100.00%> (ø)
pkg/config/config.go 63.63% <100.00%> (+8.08%) ⬆️
pkg/test/utils.go 65.00% <100.00%> (+0.29%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -143,6 +143,7 @@ func RunCommand(command string) string {
}

func DeserializeJSONToMap(t *testing.T, in string) config.GenericMap {
t.Helper()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the practical result of declaring this function as a Helper function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@KalmanMeth
By marking a function as helper, when a test fails in the helper function, the line number reported will be of the caller function rather than inside the test helper function.

Consider running the following test:

package b

import (
	"testing"
)

func Test(t *testing.T) {
	helperMarked(t, "hello", "hi") // line 8
	helperUnmarked(t, "hello", "hi")
}

func helperMarked(t *testing.T, expected, actual string) {
	t.Helper()
	if expected != actual {
		t.Errorf("expected (%v) != actual(%v)", expected, actual)
	}
}
func helperUnmarked(t *testing.T, expected, actual string) {
	if expected != actual {
		t.Errorf("expected (%v) != actual(%v)", expected, actual) // line 20
	}
}

The result is:

=== RUN   Test
    b_test.go:8: expected (hello) != actual(hi)
    b_test.go:20: expected (hello) != actual(hi)
--- FAIL: Test (0.00s)

FAIL

Copy link
Member

Choose a reason for hiding this comment

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

nice, I didn't know that

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

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

lgtm

@ronensc ronensc merged commit fb310b7 into netobserv:main Aug 3, 2022
@ronensc ronensc deleted the json-unmarshal-strict branch August 3, 2022 12:54
@jotak jotak added the breaking-change This pull request has breaking changes. They should be described in PR description. label Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This pull request has breaking changes. They should be described in PR description.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash FLP on an incorrect config file
4 participants