Add msg parameter to Equals function in testutil #398

janickic · 2018-09-27T05:48:18Z

Fixes #390
I can go through and attempt to add more meaningful messages as opposed to an empty string.

Signed-off-by: Camille Janicki [email protected]

krasi-georgiev · 2018-09-27T10:51:54Z

yeah I would say lets add some messages so I can test it locally and see how it would look like with passing and failing tests.
Thanks!

codesome

Expected values should be 2nd argument (after t). I might have missed some below.

codesome · 2018-09-27T10:55:36Z

db_test.go

@@ -110,7 +110,7 @@ func TestDataAvailableOnlyAfterCommit(t *testing.T) {
 	testutil.Ok(t, err)
 	seriesSet := query(t, querier, labels.NewEqualMatcher("foo", "bar"))

-	testutil.Equals(t, map[string][]sample{}, seriesSet)
+	testutil.Equals(t, seriesSet, map[string][]sample{}, "")


Will fix these. It looks like my code editor swapped the values

codesome · 2018-09-27T10:55:59Z

db_test.go

@@ -122,7 +122,7 @@ func TestDataAvailableOnlyAfterCommit(t *testing.T) {

 	seriesSet = query(t, querier, labels.NewEqualMatcher("foo", "bar"))

-	testutil.Equals(t, map[string][]sample{`{foo="bar"}`: {{t: 0, v: 0}}}, seriesSet)
+	testutil.Equals(t, seriesSet, map[string][]sample{`{foo="bar"}`: {{t: 0, v: 0}}}, "")


codesome · 2018-09-27T10:56:25Z

db_test.go

@@ -143,7 +143,7 @@ func TestDataNotAvailableAfterRollback(t *testing.T) {

 	seriesSet := query(t, querier, labels.NewEqualMatcher("foo", "bar"))

-	testutil.Equals(t, map[string][]sample{}, seriesSet)
+	testutil.Equals(t, seriesSet, map[string][]sample{}, "")


codesome · 2018-09-27T10:56:30Z

db_test.go

@@ -179,7 +179,7 @@ func TestDBAppenderAddRef(t *testing.T) {
 	testutil.Ok(t, err)

 	err = app2.AddFast(9999999, 1, 1)
-	testutil.Equals(t, ErrNotFound, errors.Cause(err))
+	testutil.Equals(t, errors.Cause(err), ErrNotFound, "")


codesome · 2018-09-27T10:56:56Z

db_test.go

@@ -412,7 +412,7 @@ func TestDB_Snapshot(t *testing.T) {
 		testutil.Ok(t, series.Err())
 	}
 	testutil.Ok(t, seriesSet.Err())
-	testutil.Equals(t, 1000.0, sum)
+	testutil.Equals(t, sum, 1000.0, "")


codesome · 2018-09-27T10:57:07Z

db_test.go

@@ -689,7 +689,7 @@ func TestWALFlushedOnDBClose(t *testing.T) {

 	values, err := q.LabelValues("labelname")
 	testutil.Ok(t, err)
-	testutil.Equals(t, []string{"labelvalue"}, values)
+	testutil.Equals(t, values, []string{"labelvalue"}, "")


codesome · 2018-09-27T10:59:11Z

head_test.go


 	series, ok := recs[0].([]RefSeries)
 	testutil.Assert(t, ok, "expected series record but got %+v", recs[0])
-	testutil.Equals(t, []RefSeries{{Ref: 1, Labels: labels.FromStrings("a", "b")}}, series)
+	testutil.Equals(t, series, []RefSeries{{Ref: 1, Labels: labels.FromStrings("a", "b")}}, "")


codesome · 2018-09-27T11:00:11Z

wal_test.go

-		testutil.Equals(t, byte(walSeriesSimple), flag)
-		testutil.Equals(t, []byte("Hello World!!"), b)
+		testutil.Equals(t, WALEntrySeries, et, "")
+		testutil.Equals(t, flag, byte(walSeriesSimple), "")


csmarchbanks · 2018-09-27T14:05:17Z

I took a slightly different approach (message is optional, can use variables) in a prometheus/prometheus PR (prometheus/prometheus#4639). It might be worthwhile to choose one or the other for people working in both repos.

krasi-georgiev · 2018-09-27T14:51:28Z

Had a quick look and looks good. I would say let's take your approach

janickic · 2018-09-28T02:12:51Z

@csmarchbanks Added you as a co-author because I used your code.

krasi-georgiev · 2018-09-28T08:05:13Z

ping me when it is ready for another review.

Co-authored-by: Chris Marchbanks <[email protected]> Signed-off-by: Camille Janicki <[email protected]>

janickic · 2018-10-02T03:44:52Z

@krasi-georgiev It looks like the checks are passing now. Thanks @krasi-georgiev and @codesome for suggesting to rebase.

krasi-georgiev · 2018-10-02T06:35:54Z

@codesome can you have another look and will merge.

codesome · 2018-10-02T09:12:35Z

testutil/testutil.go

 	if !reflect.DeepEqual(exp, act) {
 		_, file, line, _ := runtime.Caller(1)
-		fmt.Printf("\033[31m%s:%d:\n\n\texp: %#v\n\n\tgot: %#v\033[39m\n\n", filepath.Base(file), line, exp, act)
+		fmt.Printf("\033[31m%s:%d:\n\n\texp: %#v\n\n\tgot: %#v%s\033[39m\n\n", filepath.Base(file), line, exp, act, formatMessage(msgAndArgs))


I think it's better to print the message before exp and got. Else it might get tedious trying to get to the end and find the message. @krasi-georgiev what do you say?

yes tested it locally and you are right.

fmt.Printf("\033[31m%s:%d:%s\n\n\texp: %#v\n\n\tgot: %#v\033[39m\n\n", filepath.Base(file), line, formatMessage(msgAndArgs), exp, act)

codesome · 2018-10-02T09:24:05Z

testutil/testutil.go

+	if _, ok := msgAndArgs[0].(string); !ok {
+		return ""
+	}
+	return fmt.Sprintf(fmt.Sprintf("\n\nmsg: %s", msgAndArgs[0]), msgAndArgs[1:]...)


(nit)
As we are already checking for msgAndArgs[0] to be string, we can reuse the value from that and avoid additional fmt.Sprintf.

Maybe something like

if msg, ok := msgAndArgs[0].(string); ok { return fmt.Sprintf("\n\nmsg: "+msg, msgAndArgs[1:]...) } return ""

Signed-off-by: Camille Janicki <[email protected]>

janickic · 2018-10-03T02:52:39Z

Thanks @codesome for the review!

krasi-georgiev · 2018-10-03T08:08:42Z

Thanks! appreciated.

krasi-georgiev · 2018-10-03T08:23:53Z

@janickic here is another good issue if you are looking for more :) #237

janickic · 2018-10-08T03:34:05Z

I'll look into it! Thanks! :)

codesome suggested changes Sep 27, 2018

View reviewed changes

janickic force-pushed the camille-390 branch from 68a23d3 to 79a19c9 Compare September 28, 2018 02:12

codesome mentioned this pull request Sep 30, 2018

Add new metrics. #396

Merged

Add msg parameter to Equals function in testutil

5cb00b8

Co-authored-by: Chris Marchbanks <[email protected]> Signed-off-by: Camille Janicki <[email protected]>

janickic force-pushed the camille-390 branch from 79a19c9 to 5cb00b8 Compare October 2, 2018 03:37

codesome reviewed Oct 2, 2018

View reviewed changes

Swap print message to come before exp and got

522352a

Signed-off-by: Camille Janicki <[email protected]>

krasi-georgiev merged commit 0ce4111 into prometheus-junkyard:master Oct 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add msg parameter to Equals function in testutil #398

Add msg parameter to Equals function in testutil #398

janickic commented Sep 27, 2018

krasi-georgiev commented Sep 27, 2018

codesome left a comment

codesome Sep 27, 2018

janickic Sep 28, 2018

codesome Sep 27, 2018

codesome Sep 27, 2018

codesome Sep 27, 2018

codesome Sep 27, 2018

codesome Sep 27, 2018

codesome Sep 27, 2018

codesome Sep 27, 2018

csmarchbanks commented Sep 27, 2018 •

edited

Loading

krasi-georgiev commented Sep 27, 2018

janickic commented Sep 28, 2018

krasi-georgiev commented Sep 28, 2018

janickic commented Oct 2, 2018

krasi-georgiev commented Oct 2, 2018

codesome Oct 2, 2018

krasi-georgiev Oct 2, 2018

codesome Oct 2, 2018

krasi-georgiev Oct 2, 2018

janickic commented Oct 3, 2018

krasi-georgiev commented Oct 3, 2018

krasi-georgiev commented Oct 3, 2018

janickic commented Oct 8, 2018

Add msg parameter to Equals function in testutil #398

Add msg parameter to Equals function in testutil #398

Conversation

janickic commented Sep 27, 2018

krasi-georgiev commented Sep 27, 2018

codesome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmarchbanks commented Sep 27, 2018 • edited Loading

krasi-georgiev commented Sep 27, 2018

janickic commented Sep 28, 2018

krasi-georgiev commented Sep 28, 2018

janickic commented Oct 2, 2018

krasi-georgiev commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janickic commented Oct 3, 2018

krasi-georgiev commented Oct 3, 2018

krasi-georgiev commented Oct 3, 2018

janickic commented Oct 8, 2018

csmarchbanks commented Sep 27, 2018 •

edited

Loading