add and refactor testcase for exchange #1900

yanxuean · 2017-12-08T13:48:20Z

1.add and refactor testcase for exchange

Signed-off-by: yason yan.xuean@zte.com.cn

codecov-io · 2017-12-08T13:58:42Z

Codecov Report

Merging #1900 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #1900     +/-   ##
=========================================
+ Coverage   47.29%   47.39%   +0.1%     
=========================================
  Files          89       89             
  Lines        8830     8830             
=========================================
+ Hits         4176     4185      +9     
+ Misses       3968     3959      -9     
  Partials      686      686

Flag	Coverage Δ
#linux	`50.77% <ø> (+0.11%)`	⬆️
#windows	`42.36% <ø> (+0.11%)`	⬆️

Impacted Files	Coverage Δ
events/exchange/exchange.go	`68.91% <0%> (+6.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03bc5e9...224417a. Read the comment docs.

dnephin · 2017-12-08T15:14:34Z

events/exchange/exchange_test.go

-	testevents := []events.Event{
+	exchange := NewExchange()
+
+	t.Log("config events, All events will be published")


What's the purpose of these t.Log() messages? They read more like comments

dnephin · 2017-12-08T15:21:09Z

filters/filter_test.go

@@ -287,9 +302,25 @@ func TestFilters(t *testing.T) {
 				}
 			}

-			if !reflect.DeepEqual(results, testcase.expected) {
+			if compareEvents(results, testcase.expected) {


assert.Equal() is already being used in this repo. Probably makes more sense to use that instead of writing a custom assertion that does the same thing.

We need to compare two slice. and The element of slice has different order. So assert.Equal cann't do it.

assert.Equal(t, sorted(testcase.expected), sorted(results))

I would argue this make the expected behaviour really obvious to anyone reading the test.

The element of slice has different order.

If this is the case, something has been broken. The filtered set should have the same order as the original set. Moving away from reflect.DeepEqual here is a bug in this "refactoring".

Like I said, remove the changes to filter_test.go from this PR.

stevvooe · 2017-12-08T15:57:22Z

The testing for exchange filters is good but the refactoring of filters_test.go is completely unnecessary.

yanxuean · 2017-12-09T01:48:31Z

@stevvooe
I add a testcase for filter. The testcase is for "multi name", and the relation of multi name is ORed.
We cann't test it in the original test framework.
and The original test framework is too difficult to maintain. It depend with the order of config data.
So I think it is necessary. Please take a look at it again. Thanks.

		{
+			name: "Multi name",
+			input: []string{
+				"name==baz",
+				"name==bazo",
+			},
+			expected: []interface{}{
+				corpusS["key6"],
+				corpusS["key7"],
 			},
 		},

stevvooe · 2017-12-11T18:29:29Z

filters/filter_test.go

 			errString: `filters: parse error: [image~= >|,|< id?=?fbaq]: expected value or quoted`,
 		},
 	} {
 		t.Run(testcase.name, func(t *testing.T) {
 			t.Logf("testcase: %q", testcase.input)
-			filter, err := Parse(testcase.input)
+			var filter Filter


Completely unnecessary change here.

stevvooe · 2017-12-11T18:34:33Z

We cann't test it in the original test framework.

The case you are proposing is just two separate test cases. Its not really adding anything to the test.

If you want to test ParseAll, write a new test, but it is really not necessary, as you are just testing the compiler.

The original test framework is too difficult to maintain.

Not really. Just go based on the names.

This test also gets it wrong because it changes the type of the target from interface{} to cEntry, so the actual invariant we are checking at the end is getting broken.

yanxuean · 2017-12-12T06:15:24Z

OK, you decide it. Thanks for review @dnephin @stevvooe

stevvooe · 2017-12-12T18:59:33Z

events/exchange/exchange_test.go

@@ -17,23 +17,104 @@ import (

 func TestExchangeBasic(t *testing.T) {
 	ctx := namespaces.WithNamespace(context.Background(), t.Name())
-	testevents := []events.Event{
+	exchange := NewExchange()


Use a var block here.

Do you mean var declaration block?

var ( testevents := exchange:= xxxxx )

stevvooe · 2017-12-12T19:01:00Z

@yanxuean So, TestExchangeBasic should remain a basic test. Let's move these tests for a TestExchangeFilters test. That way, we can be sure the original test preserves the behavior.

Signed-off-by: yason <yan.xuean@zte.com.cn>

yanxuean · 2017-12-15T15:00:12Z

@stevvooe PTAL

stevvooe · 2017-12-15T21:35:36Z

LGTM

crosbymichael · 2017-12-15T22:35:10Z

LGTM

dnephin reviewed Dec 8, 2017

View reviewed changes

yanxuean force-pushed the filter branch from 25eb91c to dbc270b Compare December 9, 2017 01:55

stevvooe reviewed Dec 11, 2017

View reviewed changes

yanxuean force-pushed the filter branch from dbc270b to 66595b0 Compare December 12, 2017 06:17

yanxuean changed the title ~~add and refactor testcase for filter and exchange~~ add and refactor testcase for exchange Dec 12, 2017

stevvooe reviewed Dec 12, 2017

View reviewed changes

add testcase for event.exchange and refactor

224417a

Signed-off-by: yason <yan.xuean@zte.com.cn>

yanxuean force-pushed the filter branch from 66595b0 to 224417a Compare December 13, 2017 04:16

crosbymichael merged commit 9bb2a6f into containerd:master Dec 15, 2017

yanxuean deleted the filter branch December 15, 2017 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add and refactor testcase for exchange #1900

add and refactor testcase for exchange #1900

yanxuean commented Dec 8, 2017 •

edited

codecov-io commented Dec 8, 2017 •

edited

dnephin Dec 8, 2017

yanxuean Dec 9, 2017

dnephin Dec 8, 2017

yanxuean Dec 9, 2017

dnephin Dec 11, 2017 •

edited

stevvooe Dec 11, 2017

stevvooe commented Dec 8, 2017

yanxuean commented Dec 9, 2017 •

edited

stevvooe Dec 11, 2017

stevvooe commented Dec 11, 2017

yanxuean commented Dec 12, 2017

stevvooe Dec 12, 2017

yanxuean Dec 13, 2017 •

edited

stevvooe commented Dec 12, 2017

yanxuean commented Dec 15, 2017

stevvooe commented Dec 15, 2017

crosbymichael commented Dec 15, 2017

add and refactor testcase for exchange #1900

add and refactor testcase for exchange #1900

Conversation

yanxuean commented Dec 8, 2017 • edited

codecov-io commented Dec 8, 2017 • edited

Codecov Report

dnephin Dec 8, 2017

Choose a reason for hiding this comment

yanxuean Dec 9, 2017

Choose a reason for hiding this comment

dnephin Dec 8, 2017

Choose a reason for hiding this comment

yanxuean Dec 9, 2017

Choose a reason for hiding this comment

dnephin Dec 11, 2017 • edited

Choose a reason for hiding this comment

stevvooe Dec 11, 2017

Choose a reason for hiding this comment

stevvooe commented Dec 8, 2017

yanxuean commented Dec 9, 2017 • edited

stevvooe Dec 11, 2017

Choose a reason for hiding this comment

stevvooe commented Dec 11, 2017

yanxuean commented Dec 12, 2017

stevvooe Dec 12, 2017

Choose a reason for hiding this comment

yanxuean Dec 13, 2017 • edited

Choose a reason for hiding this comment

stevvooe commented Dec 12, 2017

yanxuean commented Dec 15, 2017

stevvooe commented Dec 15, 2017

crosbymichael commented Dec 15, 2017

yanxuean commented Dec 8, 2017 •

edited

codecov-io commented Dec 8, 2017 •

edited

dnephin Dec 11, 2017 •

edited

yanxuean commented Dec 9, 2017 •

edited

yanxuean Dec 13, 2017 •

edited