Skip to content

Restore logging of test configuration for pilot e2e tests #3459

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 23, 2018
Merged

Restore logging of test configuration for pilot e2e tests #3459

merged 2 commits into from
Feb 23, 2018

Conversation

nmittler
Copy link
Contributor

@nmittler nmittler commented Feb 13, 2018 •
edited
Loading

This was removed by #3502

To cleanly do this, I've separated out the configuration from the
test runtime environment. I've renamed Infra to Environment to
make this clear.

Because fields are now split across Config and Environment, I've
also create a new TemplateData struct that merges all the common
template parameters into a single struct. This way the template files
do not have to change. The separation of template params from the
environment should also help a bit with maintenance going forward.

@nmittler nmittler requested a review from a team February 13, 2018 22:28
@nmittler nmittler mentioned this pull request Feb 13, 2018
Merged
@ldemailly
Copy link
Member

/lgtm

@mattdelco
Copy link
Contributor

Is the --logtostderr also worthwhile to restore? Offhand I thought misc tests were passing this (and circle still is in a few places) but since everybody was I think it was migrating towards this common location in the makefile.

@istio-merge-robot
Copy link

/lgtm cancel //PR changed after LGTM, removing LGTM. @ldemailly @nmittler

@nmittler
Copy link
Contributor Author

@mattdelco

Is the --logtostderr also worthwhile to restore? Offhand I thought misc tests were passing this (and circle still is in a few places) but since everybody was I think it was migrating towards this common location in the makefile.

The CircleCI TESTOPTS already includes logtostderr. I've updated prow to include it as well.

@nmittler nmittler requested a review from a team February 14, 2018 00:05
@ldemailly
Copy link
Member

linter not happy ^

@istio-merge-robot
Copy link

@nmittler PR needs rebase

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 14, 2018
@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 14, 2018
@nmittler
Copy link
Contributor Author

@ldemailly should be good to go now ... mind re-approving?

@nmittler
Copy link
Contributor Author

/retest

@nmittler
Copy link
Contributor Author

@ldemailly any ideas why the tests are failing? Has something changed with the test environment?

@nmittler
Copy link
Contributor Author

/test istio-pilot-e2e

@ldemailly
Copy link
Member

ldemailly commented Feb 14, 2018 •
edited
Loading

@nmittler you need to urgently fix #3499
in this PR for instance

longer term there should be negative tests of the tests - ie force some failure and see that the output upon failure is reasonable. also related #3064 has never been proven / demonstrated as working

@mattdelco
Copy link
Contributor

One aspect that probably did change on you was that KUEBCONFIG wasn't being set and that was fixed about a half day ago (the earlier successful test run for this PR logged "Env variable KUBECONFIG not set. Skipping tests").

@ijsnellf
Copy link
Contributor

Let's keep this PR focused. #3499 and #3064 deserve their own PRs.

@nmittler nmittler changed the title Restoring TESTOPTS for pilot integ tests Fixing pilot e2e tests broken by recent commit Feb 15, 2018
@nmittler
Copy link
Contributor Author

@ldemailly PTAL ... I've broadened the scope of this PR to include several fixes to the pilot e2e tests

@nmittler
Copy link
Contributor Author

/test istio-pilot-e2e

@ldemailly
Copy link
Member

this still seems to be a huge diff just to change 1 logging ?
why the tutil.Infra -> tutil.Environment mass rename ?

@nmittler
Copy link
Contributor Author

@ldemailly

why the tutil.Infra -> tutil.Environment mass rename

Agreed, there are a lot of changes, but it's mostly renames. See the updated description for an overview.

The rename was effectively due the introduction of the new Config type. The term Environment seems a fairly common term to refer to the run-time environment of a testing framework, and I wanted to clearly disambiguate it from the configuration for that environment. The rename also gets us closer to the terminology used by the "common" integration testing environment.

@nmittler
Copy link
Contributor Author

@ldemailly gentle ping ... would like to get this in soonish if possible

@ldemailly
Copy link
Member

I asked Zach to have a glance

As you will own this code I suppose it is fine you do whichever you feel comfortable but I wanted a second opinion

ZackButcher
ZackButcher approved these changes Feb 23, 2018
View reviewed changes
Copy link
Contributor

@ZackButcher ZackButcher left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@istio-merge-robot
Copy link

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

@istio-merge-robot
Copy link

Automatic merge from submit-queue.

@istio-merge-robot
Copy link

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

@istio-merge-robot
Copy link

Automatic merge from submit-queue.

@istio-merge-robot
Copy link

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

@nmittler
Restore logging of test configuration for pilot e2e tests
609a97f 
This was removed by istio#3502

To cleanly do this, I've separated out the configuration from the
test runtime environment.  I've renamed `Infra` to `Environment` to
make this clear.

Because fields are now split across `Config` and `Environment, I've
also create a new `TemplateData` struct that merges all the common
template parameters into a single struct. This way the template files
do not have to change. The separation of template params from the
environment should also help a bit with maintenance going forward.
@nmittler
Copy link
Contributor Author

@ZackButcher I had to rebase ... mind re-approving?

@istio-merge-robot
Copy link

/lgtm cancel //PR changed after LGTM, removing LGTM. @ZackButcher @ldemailly @nmittler

@GregHanson
Copy link
Member

/lgtm

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GregHanson, ldemailly, ZackButcher

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

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

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
Copy link

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

@istio-merge-robot
Copy link

Automatic merge from submit-queue.

@istio-merge-robot
Copy link

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

@istio-merge-robot
Copy link

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit c44464d into istio:master Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZackButcher ZackButcher ZackButcher approved these changes

+1 more reviewer

@ldemailly ldemailly ldemailly approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@ZackButcher ZackButcher

@ldemailly ldemailly

@GregHanson GregHanson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nmittler @ldemailly @mattdelco @istio-merge-robot @ijsnellf @GregHanson @ZackButcher @googlebot @istio-testing