Migrate Pilot e2e tests to common framework. by nmittler · Pull Request #4296 · istio/istio

nmittler · 2018-03-15T18:39:49Z

No description provided.

istio-merge-robot · 2018-03-15T18:40:08Z

@nmittler PR needs rebase

codecov · 2018-03-15T19:23:36Z

Codecov Report

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

@@          Coverage Diff           @@
##           master   #4296   +/-   ##
======================================
+ Coverage      75%     75%   +1%     
======================================
  Files         297     297           
  Lines       24816   24769   -47     
======================================
- Hits        18524   18513   -11     
+ Misses       5521    5490   -31     
+ Partials      771     766    -5

Impacted Files	Coverage Δ
mixer/adapter/stackdriver/stackdriver.go	`55% <0%> (-15%)`	⬇️
pilot/pkg/config/memory/monitor.go	`82% <0%> (-9%)`	⬇️
mixer/adapter/rbac/rbac.go	`10% <0%> (-1%)`	⬇️
mixer/adapter/statsd/statsd.go	`96% <0%> (-1%)`	⬇️
pilot/pkg/serviceregistry/kube/controller.go	`67% <0%> (ø)`	⬆️
security/pkg/util/certutil.go	`100% <0%> (ø)`	⬆️
mixer/adapter/list/stringList.go	`100% <0%> (ø)`	⬆️
galley/pkg/crd/inject.go	`0% <0%> (ø)`	⬆️
mixer/adapter/servicecontrol/utils.go	`91% <0%> (+2%)`	⬆️
broker/pkg/model/config/schema.go	`40% <0%> (+3%)`	⬆️
... and 5 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 7b9fe0d...1ee38a9. Read the comment docs.

kyessenov · 2018-03-15T19:31:36Z

5k removed, 2k added. Did you forget to add something?

kyessenov · 2018-03-15T19:32:39Z

tests/util/kube_utils.go

Let's not use port-forwarding if possible. It's very brittle (socat based). Can you create an issue and add a link here.

That's fine, I'm actually not using it anyway. I originally copied mixer_test.go and thought I might need it.

costinm

Did you forget to add some files ? It looks like the tests are deleted but not added.

nmittler · 2018-03-15T20:53:01Z

@costinm all tests are now methods within pilot_test.go

costinm · 2018-03-15T21:30:50Z

Ah, github was not even showing it because it was too big...

Can you split it back or is it too hard ? Very large files are not nice, and having them grouped a bit is
easier ( and also people are already used with finding egress tests in the egress file )

I like that they are now go Tests - junit report will be great and same for the dashboard.

costinm · 2018-03-15T21:31:59Z

Also Andra had a very good point - for this kind of move it is critical to send an email to istio-dev,
and have details on how to run the tests and what to expect ( each test will be visible independently, etc).

I assume "go test -t.run=regex" will now work too ?

costinm · 2018-03-15T21:32:46Z

Deferring to Zack on the organization ( one file or many ) - treat it as a suggestion / IMHO.

costinm · 2018-03-15T21:34:32Z

Also: test is failing due to flags - I think preserving the -hub/-tag flag is good and will avoid developers pain ( if they are used with it or have scripts or IDE configs ). Easy to support too.

kyessenov · 2018-03-15T21:50:02Z

Before you attempt next refactor, I suggest to stage your changes in two steps: 1. move the files where you want them to be. 2. make changes to the files. When you do both together, github UI makes it very difficult to review.

…

On Thu, Mar 15, 2018 at 2:34 PM Costin Manolache ***@***.***> wrote: Also: test is failing due to flags - I think preserving the -hub/-tag flag is good and will avoid developers pain ( if they are used with it or have scripts or IDE configs ). Easy to support too. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4296 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJGIxov7Gbbv3JpnOUVD9NolK4J_W43Qks5tet5wgaJpZM4SsrqM> .

istio-merge-robot · 2018-03-16T08:06:35Z

@nmittler PR needs rebase

andraxylia · 2018-03-16T17:34:15Z

This PR is huge and should wait for after the release. We have other critical things to get in and this kind of churn should be done in the beginning of the release.

I see many files being removed, without understanding where the corresponding tests have been moved.

We also had a chat yesterday with where there was implication the pilot e2e tests will become the standard, and not the common tests / bookinfo @costinm @louiscryan .

nmittler · 2018-03-16T17:36:35Z

@costinm @kyessenov using a TestMain appears to require that all tests be in the same file as the TestMain. If that's the case, there doesn't appear to be a way to break this apart.

nmittler · 2018-03-16T17:51:03Z

@costinm @kyessenov I was able to separate out the tests into the original files after all. PTAL

sebastienvas · 2018-03-27T16:34:27Z

prow/istio-pilot-e2e.sh

--auth_enable=true

sebastienvas · 2018-03-27T16:34:41Z

prow/istio-pilot-e2e.sh

--auth_enable=false

sebastienvas · 2018-03-27T16:35:10Z

prow/istio-pilot-e2e-v1alpha3.sh

--auth_enable=false

sebastienvas · 2018-03-27T16:35:22Z

prow/istio-pilot-e2e-v1alpha3.sh

--auth_enable=true

sebastienvas · 2018-03-27T16:36:05Z

tests/e2e/README.md

istioctl="${GOPATH}/out/linux_amd64/release/istioctl"

istio-merge-robot · 2018-03-27T22:02:23Z

@nmittler PR needs rebase

istio-merge-robot · 2018-03-29T08:02:40Z

@nmittler PR needs rebase

sebastienvas · 2018-03-30T21:48:03Z

So you know, you actually don't need to rebase, you can just merge. github review do not work well with rebase, so you end up losing all the reviewer work.

istio-merge-robot · 2018-04-02T19:04:51Z

@nmittler PR needs rebase

ZackButcher · 2018-04-02T22:09:06Z

/lgtm

istio-testing · 2018-04-02T22:09:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, kyessenov, ZackButcher

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~.circleci/OWNERS~~ [ZackButcher,costinm]
~~prow/OWNERS~~ [ZackButcher,costinm]
~~tests/OWNERS~~ [ZackButcher,costinm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

istio-merge-robot · 2018-04-02T22:46:59Z

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

istio-merge-robot · 2018-04-02T23:54:47Z

Automatic merge from submit-queue.

nmittler requested review from a team, ZackButcher and costinm March 15, 2018 18:39

googlebot added the cla: yes label Mar 15, 2018

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 15, 2018

nmittler force-pushed the pilot_test_rewrite branch from d64bfc1 to 5fbee3c Compare March 15, 2018 18:45

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 15, 2018

nmittler force-pushed the pilot_test_rewrite branch from 5fbee3c to a524ca9 Compare March 15, 2018 19:23

kyessenov reviewed Mar 15, 2018

View reviewed changes

costinm reviewed Mar 15, 2018

View reviewed changes

costinm requested a review from andraxylia March 15, 2018 21:28

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 16, 2018

nmittler force-pushed the pilot_test_rewrite branch from 062f2f4 to d914e73 Compare March 16, 2018 17:09

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 16, 2018

nmittler force-pushed the pilot_test_rewrite branch from d914e73 to 07a61df Compare March 16, 2018 17:50

nmittler force-pushed the pilot_test_rewrite branch 2 times, most recently from 70c84d1 to 2140f35 Compare March 16, 2018 18:04

nmittler force-pushed the pilot_test_rewrite branch from c1701bc to 1369418 Compare March 27, 2018 15:58

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 27, 2018

sebastienvas reviewed Mar 27, 2018

View reviewed changes

nmittler force-pushed the pilot_test_rewrite branch from 1369418 to e1942d4 Compare March 27, 2018 16:40

mandarjog force-pushed the pilot_test_rewrite branch from e1942d4 to c712602 Compare March 27, 2018 21:40

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 27, 2018

nmittler force-pushed the pilot_test_rewrite branch from c712602 to 19b25df Compare March 27, 2018 22:52

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 27, 2018

nmittler force-pushed the pilot_test_rewrite branch from 19b25df to e9be40b Compare March 28, 2018 01:16

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 29, 2018

nmittler force-pushed the pilot_test_rewrite branch from e9be40b to 99ab4f4 Compare March 30, 2018 21:44

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 30, 2018

nmittler force-pushed the pilot_test_rewrite branch from 99ab4f4 to db8a768 Compare March 30, 2018 22:29

mandarjog mentioned this pull request Apr 2, 2018

Update Mixer Envoy config to Report Mixer requests to Mixer itself #4226

Merged

4 tasks

Migrate Pilot e2e tests to common framework.

1ee38a9

sebastienvas approved these changes Apr 2, 2018

View reviewed changes

nmittler mentioned this pull request Apr 3, 2018

Migrate Pilot e2e tests to common framework #4012

Closed

PiotrSikora mentioned this pull request Sep 11, 2018

Update Proxy SHA to latest with TCP proxy fixes. #8606

Merged

Conversation

nmittler commented Mar 15, 2018

Uh oh!

istio-merge-robot commented Mar 15, 2018

Uh oh!

codecov bot commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kyessenov commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

nmittler commented Mar 15, 2018

Uh oh!

costinm commented Mar 15, 2018

Uh oh!

costinm commented Mar 15, 2018

Uh oh!

costinm commented Mar 15, 2018

Uh oh!

costinm commented Mar 15, 2018

Uh oh!

kyessenov commented Mar 15, 2018 via email

Uh oh!

istio-merge-robot commented Mar 16, 2018

Uh oh!

andraxylia commented Mar 16, 2018

Uh oh!

nmittler commented Mar 16, 2018

Uh oh!

nmittler commented Mar 16, 2018

Uh oh!

sebastienvas Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Mar 27, 2018

Uh oh!

istio-merge-robot commented Mar 29, 2018

Uh oh!

sebastienvas commented Mar 30, 2018

Uh oh!

istio-merge-robot commented Apr 2, 2018

Uh oh!

ZackButcher commented Apr 2, 2018

Uh oh!

istio-testing commented Apr 2, 2018

Uh oh!

istio-merge-robot commented Apr 2, 2018

Uh oh!

istio-merge-robot commented Apr 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov bot commented Mar 15, 2018 •

edited

Loading

sebastienvas Mar 27, 2018 •

edited

Loading