-
Notifications
You must be signed in to change notification settings - Fork 348
Conversation
I've run all the non-slow, non-serial and non-flaky test locally, here is the test result:
Failures are mainly caused by several missing features:
|
script: | ||
- make install.deps | ||
- make test | ||
- make binaries |
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.
make test-cri will build anyway so removing make binaries just postpones the step..
Suggest keeping the name Verify.. and removing the make binaries from the 1.8.x since that will be done anyway in the test step for 1.8.3. But keep the make binaries in tip as an extra step since build and test isn't being run for it. Suggest keeping build and test since it actually does build then test...
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 feel like Build
and Test
seems better, which is more clear. :)
Build for tip
but not build for 1.8.x
makes people confusing. Build doesn't actually take too long, so an extra build seems acceptable?
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.
mostly just pointing out that deleting the -make binaries line did not remove the call to make binaries. Yeah an extra build is acceptable since it's so fast. Just pointing out it's also unnecessary on the 1.8 build because you'll be doing it twice... not sure why it's confusing to have a build in a verify step but not verify in a build step :-)
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.
See comments. Overall great new feature!
.travis.yml
Outdated
go: 1.8.x | ||
- script: |
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.
ok sure, not doing the long test twice is ok, the verify & build step for tip should be enough to make sure most of the golang issues are addressed. Will only miss runtime issues.
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.
ACK.
verify: lint gofmt boiler | ||
|
||
version: | ||
@echo $(VERSION) | ||
|
||
lint: check-gopath |
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.
was only necessary to do this test prior to go 1.8 and we require 1.8 so.. yeah ok to remove :-) Maybe to a go version check?
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.
The problem is that check-gopath
is a PHONY,, target depends on it will run every time.
I want to avoid rebuild, if the binaries are built and source code is not changed, so I have to remove this.
clean: | ||
rm -f $(BUILD_DIR)/cri-containerd | ||
rm -rf $(BUILD_DIR)/* |
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.
drf?
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.
d
seems not required if r
is specified.
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.
d is even if the directory is empty..
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.
IIUC, r
is a superset of d
. :)
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.
interesting.. from quick test I see you are correct. Thx
@./hack/test-cri.sh | ||
|
||
test-e2e-node: binaries |
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.
need to add test-e2e-node to the above help section...
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.
Done.
@@ -0,0 +1,53 @@ | |||
#!/bin/bash |
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.
looks good
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
set -o nounset |
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.
# prevent unset variables
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.
All script has this, don't think we want to comment all of them. :)
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
set -o nounset | ||
set -o pipefail |
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.
# force script to exit if any command in a pipeline errors
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.
ditto.
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.
:)
export SKIP=${SKIP:-${DEFAULT_SKIP}} | ||
REPORT_DIR=${REPORT_DIR:-"/tmp/test-e2e-node"} | ||
|
||
if [[ -z "${GOPATH}" ]]; then |
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.
That's ironic :-)
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.
See comment above.
21855e6
to
c3ebd78
Compare
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.
/LGTM your call on the nits.
Cheers!
@mikebrow Let's wait for the test to finish, till know everything looks good. :) @kubernetes-incubator/maintainers-cri-containerd |
Travis is in a bad state. The last test seems to have over run the log max of 4m and CI was not successful in stopping the jobs. |
@mikebrow Yeah, we are printing log into travis test output directly. It seems that we have to upload them somewhere else. |
56199a0
to
c8814d2
Compare
@mikebrow Separated the e2e test into another stage, and only print out cri-containerd log for now. |
.travis.yml
Outdated
- script: | ||
- stage: E2E Test | ||
script: | ||
# TODO(random-liu): Uncomment after test passes. |
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.
What's the cron job schedule?
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.
Daily for now. There are only 3 options: daily, weekly and monthly.
.travis.yml
Outdated
- make test-e2e-node | ||
after_script: | ||
# TODO(random-liu): Upload log to GCS. | ||
- test -f /tmp/test-e2e-node/cri-containerd.log && cat /tmp/test-e2e-node/cri-containerd.log |
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.
probably a todo or issue here to figure out how to filter the logs a bit more so there's room in our 4m limit to get the containerd log...
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.
@mikebrow I've changed the log level, :)
I think the best solution is to upload the log onto GCS. Or run the test in kubernetes test_infra, which I'm also working on recently.
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.
/LGTM just a nit for a todo comment and a question
All pass so good to remove comments from if not cron job skip for the e2e bucket :-) |
seems that some network related test fails. We need to figure it out. :)
…On Aug 21, 2017 12:00 PM, "Mike Brown" ***@***.***> wrote:
All pass so good to remove comments from if not cron job skip for the e2e
bucket :-)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#145 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFjVuzO9H8Fa8Qi6sk9nS8n226swzR7Dks5sadPKgaJpZM4O8i3l>
.
|
@mikebrow Actually, there are still test failing which succeed in my environment:
Because I didn't return that error code, the test passes. However, I thought it was OK. Let's get the test running first, and then fix these test failures. @mikebrow |
c8814d2
to
6694e4b
Compare
6694e4b
to
b860d56
Compare
Signed-off-by: Lantao Liu <lantaol@google.com>
b860d56
to
c05a7e7
Compare
Add node e2e test CI.
This PR:
Please note that:
TRAVIS_EVENT_TYPE
related code for now, so that node e2e test will still be triggered in this PR. I'll uncomment that after node e2e passes./cc @kubernetes-incubator/maintainers-cri-containerd
Signed-off-by: Lantao Liu lantaol@google.com