Integration framework improvement by yutongz · Pull Request #1936 · istio/istio

yutongz · 2017-11-30T17:23:51Z

What this PR does / why we need it:
Some improvements based on the prototype before.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

none

Introduce common components and env
Introduce entry script
Add flags instead of hardcoded binary path.

Also include a doc about steps to adding tests: https://goo.gl/fej8qb
For design and concept, please refer https://goo.gl/1zE5Rq
Will create a github README page after this PR merged.

For now, you can checkout my branch and run tests/integration/integration.sh on a machine where you can run envoy binary.

/cc @wattli @mandarjog @douglas-reid

codecov · 2017-11-30T17:59:55Z

Codecov Report

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

@@            Coverage Diff            @@
##           master   #1936      +/-   ##
=========================================
+ Coverage   81.66%     82%   +0.34%     
=========================================
  Files         190     190              
  Lines       15972   15624     -348     
=========================================
- Hits        13043   12812     -231     
+ Misses       2462    2359     -103     
+ Partials      467     453      -14

Flag	Coverage Δ
#broker	`47.27% <ø> (ø)`	⬆️
#mixer	`83.59% <ø> (+0.49%)`	⬆️
#pilot	`80.85% <ø> (+0.19%)`	⬆️
#security	`92.03% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
mixer/adapter/svcctrl/handler.go	`27.77% <0%> (-10.94%)`	⬇️
mixer/adapter/prometheus/prometheus.go	`75.24% <0%> (-3.79%)`	⬇️
mixer/adapter/denier/denier.go	`88.88% <0%> (-3.22%)`	⬇️
mixer/adapter/svcctrl/reportbuilder.go	`86.46% <0%> (-2.11%)`	⬇️
mixer/adapter/svcctrl/svcctrl.go	`61.11% <0%> (-1.26%)`	⬇️
pilot/proxy/envoy/discovery.go	`75.93% <0%> (-0.56%)`	⬇️
mixer/adapter/stdio/stdio.go	`98.38% <0%> (-0.53%)`	⬇️
mixer/cmd/client/cmd/util.go	`63.39% <0%> (ø)`	⬆️
pilot/model/validation.go	`92.74% <0%> (ø)`	⬆️
mixer/adapter/svcctrl/client.go	`0% <0%> (ø)`	⬆️
... and 23 more

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 2f58299...02f3dfb. Read the comment docs.

yutongz · 2017-11-30T18:47:48Z

/cc @hklai

hklai · 2017-12-01T01:47:08Z

tests/integration/component/fortio_server/fortio_server_comp.go

Is there a need for an additional LocalComponent if it is the same as CommonProcessComp?

Also, perhaps you can just rename CommonProcessComp to LocalProcessComp?

hklai · 2017-12-01T03:59:43Z

tests/integration/component/fortio_server/fortio_server_comp.go

Sorry I know I sent you this way before, but now I am having a second though looking at the outcome. =p

Having LocalComponent taking another Component as inheritance/extension is really ugly.

Shall we rename CommonProcessComp to just Process which represents a Process owned by this Component instead?

We may as well ditch the whole inheritance paradigm here.

I guess you are right. The current solution is a little complex. Right, we can have a Process[struct] that wraps a os.Process and have start() and stop() to start/stop a background process. And those methods can be used in Component.Start()/Stop()

yutongz · 2017-12-04T20:17:33Z

PTAL

hklai · 2017-12-04T23:51:02Z

tests/integration/component/fortio_server/fortio_server_comp.go

-	name    string
-	process *os.Process
-	logFile string
+	framework.CompProcess


How about just Process?

hklai · 2017-12-04T23:56:37Z

tests/integration/framework/testEnvManager.go

-	TestID     string
-	Components []Component
+// TestEnvManager is core test framework struct
+type TestEnvManager struct {


How about EnvironmentStarter?

Other than starting, this does not manage much right?

Personally, I prefer "manager" or may be some other words since it takes care of the whole cycle of a environment: start, stop, alive check.

And I plan to add a parallel health monitor for the environment such that when tests failed in the middle we can see if the component crash is the reason.

hklai · 2017-12-04T23:57:06Z

tests/integration/framework/testEnvManager.go

 // SetUp sets up the whole environment as well brings up components
-func (framework *IstioTestFramework) SetUp() (err error) {
-	if err = framework.TestEnv.Bringup(); err != nil {
+func (envManager *TestEnvManager) SetUp() (err error) {


Should this be StartUp() instead?

hklai · 2017-12-04T23:58:35Z

tests/integration/framework/testEnvManager.go

+	envManager.TestEnv.Cleanup()
 }

 // IsEnvReady check if the whole environment is ready for running tests


Please document that this method wait for the env to be ready.

Probably rename this to waitUntilReady or something

hklai · 2017-12-05T00:00:08Z

tests/integration/framework/process.go

+
+// CompProcess is a wrap of os.Process
+// With implemented methods to control local components
+type CompProcess struct {


Maybe TestProcess instead?

hklai · 2017-12-05T01:37:08Z

tests/integration/framework/testEnvManager.go

+// TestEnvManager is core test framework struct
+type TestEnvManager struct {
+	TestEnv     TestEnv
+	TestID      string


Why is TestID needed?

For identification in log/test summary

hklai

Looks fine to me. Thanks!

hklai · 2017-12-05T22:25:57Z

tests/integration/component/proxy/proxy_binary_comp.go

-	name    string
-	process *os.Process
-	logFile string
+	testProcess framework.TestProcess


Where is testProcess assigned?

hklai · 2017-12-05T22:28:04Z

tests/integration/example/environment/appOnlyEnv/app_only_env.go

-// Bringup doing setup for AppOnlyEnv
+// Bringup implement the function in TestEnv
+// Create local temp dir. Can be manually overrided if necessary.
 func (appOnlyEnv *AppOnlyEnv) Bringup() (err error) {


Can we have consistent names for start up and shut down between Env and Comp (or even Process)?

hklai · 2017-12-06T00:09:37Z

/lgtm

istio-merge-robot · 2017-12-06T00:09:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hklai

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [hklai]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2017-12-06T00:35:24Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-12-06T01:01:38Z

Automatic merge from submit-queue.

mangchiandjjoe · 2017-12-12T01:02:34Z

tests/integration/framework/component.go

@@ -20,7 +20,7 @@ type Component interface {
 	GetName() string

 	// Bringup doing setup for this component


The comment seems to be in a wrong place

mangchiandjjoe · 2017-12-12T01:02:36Z

tests/integration/framework/component.go


 	// Bringup doing setup for this component
-	// Start() is being called in framework.SetUp()
+	// Start() is being called in framework.StartUp()


Can we remove () in the comment of Start and Stop?

yutongz · 2017-12-12T01:38:09Z

@mangchiandjjoe Will resolve them in my next PR, thanks!

yutongz added 9 commits November 28, 2017 09:23

WIP

9288332

WIP

7f68a75

common comp

f4cef53

WIP

07d025f

common comp for fortio & mixer

bc6a267

pass binary path through flag

b26feb8

move common items under framework

834a874

Add config interface

e10cb19

Add skip_cleanup flag

a39e2f6

istio-testing added the release-note-none label Nov 30, 2017

googlebot added the cla: yes label Nov 30, 2017

fix unittest failure

5a62113

istio-merge-robot added the do-not-merge/post-submit label Nov 30, 2017

istio-testing requested a review from hklai November 30, 2017 18:47

istio-merge-robot added the do-not-merge/post-submit label Nov 30, 2017

yutongz requested review from douglas-reid, mandarjog and wattli November 30, 2017 23:12

istio-merge-robot added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 30, 2017

hklai reviewed Dec 1, 2017

View reviewed changes

istio-merge-robot added do-not-merge/post-submit and removed do-not-merge/post-submit labels Dec 1, 2017

yutongz added 2 commits December 2, 2017 10:58

WIP

faf51c1

remove heritance, add main entry to env

7a5f127

yutongz force-pushed the integration branch from 54ba4e0 to 7a5f127 Compare December 4, 2017 16:04

istio-merge-robot added the do-not-merge/post-submit label Dec 4, 2017

rename framework to testEnvManager

33283bc

hklai reviewed Dec 5, 2017

View reviewed changes

istio-merge-robot removed the do-not-merge/post-submit label Dec 5, 2017

hklai reviewed Dec 5, 2017

View reviewed changes

istio-merge-robot added do-not-merge/post-submit PostSubmit Failed/Contact Oncall and removed do-not-merge/post-submit PostSubmit Failed/Contact Oncall labels Dec 5, 2017

rename and remove inheritance from TestProcess

02f3dfb

hklai approved these changes Dec 5, 2017

View reviewed changes

istio-merge-robot removed the PostSubmit Failed/Contact Oncall label Dec 5, 2017

istio-testing assigned hklai Dec 6, 2017

istio-testing added the lgtm label Dec 6, 2017

istio-merge-robot added the approved label Dec 6, 2017

istio-merge-robot merged commit e7d8f7d into istio:master Dec 6, 2017

mangchiandjjoe reviewed Dec 12, 2017

View reviewed changes

wattli mentioned this pull request Dec 12, 2017

New integration test framework for security #2091

Merged

yutongz deleted the integration branch February 5, 2018 19:11

		@@ -20,7 +20,7 @@ type Component interface {
		GetName() string

		// Bringup doing setup for this component

Conversation

yutongz commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yutongz commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yutongz commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hklai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hklai commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yutongz commented Dec 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yutongz commented Nov 30, 2017 •

edited

Loading

codecov bot commented Nov 30, 2017 •

edited

Loading