-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Initial support for extended integration test #1230
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
Conversation
|
@bparees @csrwng @soltysh PTAL To execute extended tests, you have to: I don't like that I need to start the OpenShift server (all-in-one), since that breaks the parallelization of tests (I dunno if it is good idea to have multiple OpenShift server/etcd/... running simultaneously)... The second thing I don't like is that I can't verify the application is working without having Docker registry in place... Right now, the build does not have any Output defined and I just assume that when it finishes as 'Complete' there was no error during the STI build.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI. this should be probably solved by using Watch() instead of polling, I just need to get familiar with the Watch() itself first...
Could we start the all-in-one server, set it up, then run all tests in parallel (each using its own namespace)?
I was under the impression that what we wanted was a go version of the e2e tests with everything setup, including the registry. (so e2e extended posing as an integration test) |
|
@csrwng if extended tests will be a new set of tests, then I don't think I can re-use the code in integration... (dunno if I can just import the test_server.go ;-) |
|
That's what I thought we were going to do :-) |
|
@csrwng I can still move this into its own package :-) (this is the feedback I was hoping for :-) |
You have my vote |
|
@csrwng so the command to launch the tests is now: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having those bits and pieces here and there, let's create a test/util package and put there all the utility stuff such as RequireDocker, RequireEtcd, etc.
we should add a hack/test-extended.sh to run it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are overusing those, especially in that we do copy&paste for other integration tests. I think it would make sense to clear this out doing this. Since for this test you are requiring etcd and docker this line should state:
// +build integration,extended,etcd,docker
IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be no tags at all here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may want to leave integration,extended since you're passing those with OS_TEST_TAGS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it just be extended? these tests aren't run when hack/test-integration.sh is run (because they're in a different package), so tagging them as integration is a bit confusing.
|
🌻 for the idea itself |
+1 Maybe call it |
|
I think it's reasonable to start the registry service as part of the basic integration test environment setup. and i agree w/ @csrwng that we should be able to run in parallel by using appropriate namespaces (though that probably means we need some more generic framework that will dynamically generate unique namespaces and substitute it into templates/etc, otherwise we're relying on everyone to create their tests using unique namespaces). also currently namespaces don't work for services...upstream issue still pending. |
|
might be useful to have a soft-reset for openshift... ie "delete all pods/etc, clean up etcd (maybe just cleaning up etcd is sufficient), stop all docker containers" might be faster than starting openshift again for each test. though given that you'd need to purge/restart the registry too i guess it probably wouldn't save much time.....i just worry about how long each integration test is going to take if we have to start openshift and wait for the registry to deploy....that's like 60s just in startup time per test. I know part of the point of this is to have a place for longer tests to live, but still. |
|
I guess then it'll be simpler and faster at the same time, to generate new namespace for each test. Eg. we could be generating env var called |
c30caa4 to
19159d0
Compare
hack/test-integration.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we have that verbosity connected with VERBOSE env var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you need the build to be verbose?
d661722 to
da2b5b3
Compare
|
FYI, now you execute the tests using: (you can optionally set VERBOSE=1 before you run that command to get more verbose output...) |
hack/test-extended.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ${TMPDIR:-/tmp} instead of /tmp would be useful in case you want to use a different temp directory
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1375/) |
25c4464 to
443700e
Compare
|
@deads2k can you please check the latest failure? I think it has something to do with Policies vs Namespaces but I can't figure out what broke there.... |
3e1bf88 to
153b70d
Compare
|
@deads2k this might be related: |
|
Also I wonder why TestDNS requires Docker: |
50e8fd0 to
864bedb
Compare
864bedb to
47caf77
Compare
|
LGTM but it looks like integration tests are still failing on go 1.4 |
|
[merge] (will fix all issues as follow up) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1223/) (Image: devenv-fedora_1081) |
|
[merge] again.. seems like random failure |
|
Evaluated for origin up to 47caf77 |
Merged by openshift-bot
This is highly WIP atm. I have this here just to gather feedback and thoughts ;-)