Skip to content
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.

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

e2e tag cleanup #17180

Merged
merged 2 commits into from
Nov 24, 2015
Merged

Conversation

karlkfi
Copy link
Contributor

@karlkfi karlkfi commented Nov 12, 2015

  • Skip [Skipped] tests during conformance testing
  • Remove "Suite" from e2e tag names
  • Move group type e2e tags to the front of names, and to as high a context as applicable
  • Move Conformance e2e tags to the end of test names (test specific)
  • Move Skipped e2e tags to the end of names, but to as high a context as applicable
  • Move ClusterDns e2e test to its own file (because it's not skipped like the others)
  • Group FSGroup e2e tests

@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 12, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XL

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 12, 2015

No new tests here, just refactored existing tests, mostly their names and contexts/describes.

@k8s-bot
Copy link

k8s-bot commented Nov 12, 2015

GCE e2e test build/test passed for commit da94bdbb22a5613a3e26a47d18049b7711d9d54b.

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 12, 2015

Blocked by #17054

@ghost
Copy link

ghost commented Nov 12, 2015

Hurrah! Thanks for taking on this much-needed tidy-up @karlkfi

@jdef
Copy link
Contributor

jdef commented Nov 13, 2015

👍 ditto -- glad to see the cleanup

@@ -34,7 +34,7 @@ const (
scaleDownTimeout = 30 * time.Minute
)

var _ = Describe("Autoscaling", func() {
var _ = Describe("[Autoscaling] [Skipped]", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not "Autoscaling" without the quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The [Autoscaling] tag used in other tests, not just this describe block. So I wanted to be consistent in case people filter with the brackets. It's a subtle difference between a group tag used for filtering and a describe/context block just used for abstraction.

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 13, 2015

Rebased to get fix from #17054

@quinton-hoole you got a second to officially review this? Need a googler sign off.

@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e test build/test passed for commit 9af8a01a819ad05aefe09a5d3946206782369392.

@@ -162,21 +162,18 @@ var _ = Describe("Density", func() {
// (metrics from other tests affects this one).
// TODO: Reenable once we can measure latency only from a single test.
Copy link
Member

Choose a reason for hiding this comment

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

it seems like we should either remove this TODO or leave the option to make these tests unskipped by default? @fgrzadkowski

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were all being skipped. So I removed the functionality to skip them individually and skipped the whole block.
You can still unskip them by specifying them as a group or individually.

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 14, 2015

Once this PR is merged, the Mesos/Docker cluster will be 100% conformant on master.
The 4 FSGroup tests are the only ones still failing: https://teamcity.mesosphere.io/viewLog.html?buildId=75719&tab=buildResultsDiv&buildTypeId=Oss_KubernetesMesos_ConformanceTests&guest=1

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 14, 2015

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2015
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2015
@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 17, 2015

Rebased to resolve conflicts:

  • Horizontal pod autoscaling for deployments is now marked [Autoscaling]
  • Example tests now use runKubectlOrDie instead of runKubectl

@k8s-bot
Copy link

k8s-bot commented Nov 17, 2015

GCE e2e build/test failed for commit cb928f1e114ec00dbb560b9fd8153c31f2940ae1.

@k8s-bot
Copy link

k8s-bot commented Nov 17, 2015

GCE e2e test build/test passed for commit 7d7fb77367e5853ae8221270714c9275dcf44af1.

@k8s-bot
Copy link

k8s-bot commented Nov 17, 2015

GCE e2e build/test failed for commit f597fb9540e0073b5b6d5fe5ee3b86609cbe0bb5.

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 17, 2015

That e2e is very not happy... a million timed out waiting for the condition...

@ixdy
Copy link
Member

ixdy commented Nov 18, 2015

lgtm, please squash

@sttts sttts deleted the skipped-cleanup branch November 24, 2015 08:44
@piosz
Copy link
Member

piosz commented Nov 24, 2015

@karlkfi @ixdy
This PR disabled some important tests in Autoscaling suite. This change may also break some other suites (please check it). Please be really, really careful when merging such change.

@wojtek-t
Copy link
Member

This PR also change what is running in scalability suite. I'm fixing it in #17714

+1000 to what @piosz said - we don't want to spend hours on fixing problems, especially, if those are cleanup changes that doesn't introduce any new features...

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 24, 2015

@wojtek-t As you can see from the change log of this PR, the Latency test was already marked as a part of the [Performance Suite]. It's not new. The only difference was that it renamed the tags to remove the "Suite", and then renamed the focus/skip ginkgo flags to use the full tag with \[Performance\]. How did that change what was running?

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 24, 2015

@piosz That looks like an oversight on my part when I rebased to include the Horizontal pod autoscaling for deployments as well as for replication controllers (mentioned in #17180 (comment)). It must have gotten reverted in one of the subsequent rebases, and then the record lost in a squash. However, your fix is incomplete. The Deployment describe block should now also be marked [Autoscaling], which means that actually the parent describe block should have the tag.

@wojtek-t
Copy link
Member

@karlkfi - there must have been some configuration detail, because the latency test was NOT running in our scalability suite

@ikehz
Copy link
Contributor

ikehz commented Nov 24, 2015

We're also running into problems with porting this to the release-1.1 branch. Our tests on the release branch are now confused because not everything is properly labeled. @karlkfi, can you cherry pick it, (use hack/cherry_pick_pull.sh,) and resolving conflicts?

cc @kubernetes/goog-testing

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 24, 2015

@ihmccreery are you running the master:head test scripts against the release branch? Why not use the scripts from the branch you're testing?

@ikehz
Copy link
Contributor

ikehz commented Nov 24, 2015

Yes, we run the master hack/jenkins/e2e.sh config script on master, but do run tests against old branches. hack/jenkins/e2e.sh is supposed to be version agnostic, but isn't really, especially with changes like this that redefine how we run tests.

@ixdy
Copy link
Member

ixdy commented Nov 24, 2015

OK, so is someone owning fixing the release-1.1 branch? I'm not sure if that's something to discuss in #17734 or elsewhere.

Auditing tests right now on kubernetes-e2e-gce-release-1.1. We're now running the following tests (which we weren't before):

  • Skipped Cluster upgrade kube-push of master should maintain responsive services
  • Skipped Cluster upgrade upgrade-cluster should maintain a functioning cluster
  • Skipped Cluster upgrade upgrade-master should maintain responsive services

Unsurprisingly, this is breaking a lot of the other tests.

@ixdy
Copy link
Member

ixdy commented Nov 24, 2015

Good news is that the builds running against master all appear to be working correctly.

@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 24, 2015

Well, I guess it's a good thing this came up, even if it did so in a somewhat painful way. Running unversioned test scripts against versioned tests is a good way to not be able to make any changes... I was unaware that was happening and didn't really have any way of knowing without access to your jenkins.

The obvious solution is to run the tests on the branch you're testing. We're doing this in the Mesosphere CI, with some exception code that can override the CI scripts with ones from a specific repo/branch in case we need to override the scripts from another branch.

Is there some reason the google jenkins can't do this?

@ixdy
Copy link
Member

ixdy commented Nov 24, 2015

Let's take this discussion to #17734 perhaps?

@ixdy
Copy link
Member

ixdy commented Nov 24, 2015

I just created #17735 and #17736 to fix the "Skipped" tag on release-1.0 and release-1.1, which should fix our issues with kubernetes-e2e-gce-release-1.1. It's not a full cherrypick of this PR because I don't want to pull in all of the other changes.

karlkfi pushed a commit to mesosphere-backup/kubernetes that referenced this pull request Dec 14, 2015
- Skip [Skipped] tests when testing for conformance kubernetes#17180
- Add cluster/test-conformance.sh script kubernetes#17997
- Support passing args to cluster/test-smoke.sh kubernetes#15404
- Support annotated tags in conformance test version detection kubernetes#17839
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
- Skip [Skipped] tests when testing for conformance kubernetes#17180
- Add cluster/test-conformance.sh script kubernetes#17997
- Support passing args to cluster/test-smoke.sh kubernetes#15404
- Support annotated tags in conformance test version detection kubernetes#17839
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
- Skip [Skipped] tests when testing for conformance kubernetes#17180
- Add cluster/test-conformance.sh script kubernetes#17997
- Support passing args to cluster/test-smoke.sh kubernetes#15404
- Support annotated tags in conformance test version detection kubernetes#17839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test area/test-infra lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet