Skip to content

Fix broken tests; make sure we don't mask travis script exit codes#2274

Merged
dmcgowan merged 2 commits intocontainerd:masterfrom
estesp:debug-travis
Apr 4, 2018
Merged

Fix broken tests; make sure we don't mask travis script exit codes#2274
dmcgowan merged 2 commits intocontainerd:masterfrom
estesp:debug-travis

Conversation

@estesp
Copy link
Copy Markdown
Member

@estesp estesp commented Apr 4, 2018 •
edited
Loading

Fix the tests that were using an invalid container ID and fix travis script to not mask error exit return codes.

Signed-off-by: Phil Estes [email protected]

@estesp estesp changed the title [CI DEBUG] Test whether use of exit is hiding errors Fix broken tests; make sure we don't mask travis script exit codes Apr 4, 2018
@estesp
Copy link
Copy Markdown
Member Author

estesp commented Apr 4, 2018

This should actually be reviewed and merged now if tests pass.

@estesp
Fix tests using invalid ID
b769cce 
These tests are using their name as ID, but subtests add a forward slash
connected to the parent test, and slash (/) is an invalid character for
container IDs.

Signed-off-by: Phil Estes <[email protected]>
@estesp
Copy link
Copy Markdown
Member Author

estesp commented Apr 4, 2018

This looks better:

--- PASS: TestContainerUser (2.60s)
    --- PASS: TestContainerUser/UserNameAndGroupName (0.98s)
    --- PASS: TestContainerUser/UserIDAndGroupName (0.80s)
    --- PASS: TestContainerUser/UserNameAndGroupID (0.51s)
    --- PASS: TestContainerUser/UserIDAndGroupID (0.31s)
PASS

@estesp estesp added this to the 1.1 milestone Apr 4, 2018
mlaventure
mlaventure reviewed Apr 4, 2018
View reviewed changes
Comment thread .travis.yml
test $exit_code -ne 0 && cat /tmp/containerd-cri.log ;
test $? -ne 0 && cat /tmp/containerd-cri.log ;
sudo pkill containerd ;
exit $exit_code ;
Copy link
Copy Markdown
Contributor

@mlaventure mlaventure Apr 4, 2018

Choose a reason for hiding this comment

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

What will be the exit code now? What's going on in Travis? 😕

Copy link
Copy Markdown
Member Author

@estesp estesp Apr 4, 2018

Choose a reason for hiding this comment

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

Turns out travis is already collecting exit codes from each command and will exit with failure if any of the commands within script: have a non-zero return code. Somehow, calling exit circumvents the rest of travis processing--you can see from older builds that travis never even got to output its usual "closing" success or fail when the exit $exit_code ; call exists in travis.yml! basically the collection of this exit code was unnecessary as travis is already handling that.

Copy link
Copy Markdown
Member Author

@estesp estesp Apr 4, 2018

Choose a reason for hiding this comment

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

Documented here: https://docs.travis-ci.com/user/customizing-the-build/#How-does-this-work%3F-(Or%2C-why-you-should-not-use-exit-in-build-steps)

Copy link
Copy Markdown
Contributor

@mlaventure mlaventure Apr 4, 2018

Choose a reason for hiding this comment

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

Ah! makes sense now, thanks :)

mlaventure
mlaventure approved these changes Apr 4, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 4, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2274      +/-   ##
==========================================
+ Coverage   41.06%   45.39%   +4.32%     
==========================================
  Files          66       83      +17     
  Lines        7775     9173    +1398     
==========================================
+ Hits         3193     4164     +971     
- Misses       4077     4332     +255     
- Partials      505      677     +172
Flag Coverage Δ
#linux 49.92% <ø> (?)
#windows 41.06% <ø> (ø) ⬆️
Impacted Files Coverage Δ
mount/temp_unix.go 0% <0%> (ø)
oci/spec_unix.go 98.37% <0%> (ø)
mount/mount.go 0% <0%> (ø)
mount/mount_linux.go 0% <0%> (ø)
snapshots/overlay/check.go 0% <0%> (ø)
snapshots/overlay/overlay.go 52.52% <0%> (ø)
mount/temp.go 0% <0%> (ø)
content/local/store_unix.go 75% <0%> (ø)
archive/time_unix.go 71.42% <0%> (ø)
mount/lookup_unix.go 0% <0%> (ø)
... and 14 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 ad2548a...b769cce. Read the comment docs.

dmcgowan
dmcgowan approved these changes Apr 4, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan dmcgowan merged commit 4913969 into containerd:master Apr 4, 2018
@estesp estesp deleted the debug-travis branch April 4, 2018 20:26
@Random-Liu Random-Liu mentioned this pull request Jul 31, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@mlaventure mlaventure mlaventure approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.1

Development

Successfully merging this pull request may close these issues.

4 participants

@estesp @codecov-io @dmcgowan @mlaventure