BATS tests - lots of mostly minor cleanup - #1935
Closed
edsantiago wants to merge 4 commits into
Closed
Conversation
Contributor
Author
|
I don't think this is going to pass tests: I think there's some line-count tests that are failing due to removing --debug, but I can't be sure because there are other problems affecting my test runs (i.e. buildah not working at all on f31/rawhide, and f30 version being incompatible with current tests). And my VM died and I won't be able to get back to this next week. So let's just see what CI has to say, and I'll take a fresh look at this next week. (And, if there's strong objection to my removing |
Contributor
|
@edsantiago thanks for the PR. I didn't dive in deeply as the CI tests failed as you expected. However in general the changes LGTM. I'm fine with the --debug removal. I've actually struggled a few times with too much output from a failing test during development. |
Contributor
|
@edsantiago Still working on this? |
Contributor
Author
|
Oh yes. It's slow and painstaking but I've fixed several issues including a bug in the tests themselves. Still not running clean all the way through. |
First, and possibly most controversial, remove --debug flag. Output on test failure is painful to read. It is unlikely (but not inconceivable) that --debug will be of any use. Second, fix a failure message so it properly uses %s instead of string interpolation. (If the test name itself includes a percent sign, we get undefined behavior on output). Fix a few instances of 'buildah' to be 'run_buildah'. Remove some unnecessary 'run/echo/check-status' sequences, sticking with the much cleaner 'test'. When BATS shows an error of the form 'this failed: [ $status -eq 0 ]' I weep in despair. Signed-off-by: Ed Santiago <santiago@redhat.com>
Well, yikes: fix a broken file-mode check in two 'add' tests. They were expecting 755, but the actual mode is 644. --debug was spitting out unrelated output, including a 755 that the test was matching as a substring. Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago
force-pushed
the
bats_cleanup
branch
2 times, most recently
from
October 29, 2019 21:03
0f5fd79 to
b0ed947
Compare
- add explicit --log-level=debug in some buildah-bud invocations where we need to check debug output - use expect_line_count() in one place, to get better diagnostics - formats test: refactor, make more readable - imgtype invoker: remove -debug - fix one more instance of 'step' needing to be 'STEP' - fix one mkdir to mkdir -p, and run rm -rf beforehand, to deal with a failed prior run that might have left state Signed-off-by: Ed Santiago <santiago@redhat.com>
The newer-than and older-than test was failing (desired result) but it's because the command had a syntax error (not desired)! Rewrite that section a little, making it more robust (actually confirming that mtime is preserved, versus simple mtime match because of time of execution) and also with more readable diagnostic in case of error. Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago
force-pushed
the
bats_cleanup
branch
from
October 30, 2019 00:21
b0ed947 to
58ee138
Compare
Contributor
Author
|
Whew - tests are finally green. It is quite a bit changed since the first iteration; most of the new work is in separate commits. |
vrothberg
reviewed
Oct 30, 2019
vrothberg
left a comment
vrothberg
left a comment
Contributor
There was a problem hiding this comment.
Wow, nice work @edsantiago!
LGTM
Contributor
Contributor
|
let's ship it! LGTM |
|
📌 Commit 58ee138 has been approved by |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 30, 2019
First, and possibly most controversial, remove --debug flag. Output on test failure is painful to read. It is unlikely (but not inconceivable) that --debug will be of any use. Second, fix a failure message so it properly uses %s instead of string interpolation. (If the test name itself includes a percent sign, we get undefined behavior on output). Fix a few instances of 'buildah' to be 'run_buildah'. Remove some unnecessary 'run/echo/check-status' sequences, sticking with the much cleaner 'test'. When BATS shows an error of the form 'this failed: [ $status -eq 0 ]' I weep in despair. Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: #1935 Approved by: giuseppe
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 30, 2019
Well, yikes: fix a broken file-mode check in two 'add' tests. They were expecting 755, but the actual mode is 644. --debug was spitting out unrelated output, including a 755 that the test was matching as a substring. Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: #1935 Approved by: giuseppe
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 30, 2019
- add explicit --log-level=debug in some buildah-bud invocations where we need to check debug output - use expect_line_count() in one place, to get better diagnostics - formats test: refactor, make more readable - imgtype invoker: remove -debug - fix one more instance of 'step' needing to be 'STEP' - fix one mkdir to mkdir -p, and run rm -rf beforehand, to deal with a failed prior run that might have left state Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: #1935 Approved by: giuseppe
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 30, 2019
The newer-than and older-than test was failing (desired result) but it's because the command had a syntax error (not desired)! Rewrite that section a little, making it more robust (actually confirming that mtime is preserved, versus simple mtime match because of time of execution) and also with more readable diagnostic in case of error. Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: #1935 Approved by: giuseppe
|
💔 Test failed - status-travis |
Contributor
Author
|
Test failure looks like a transient: /retest |
Contributor
|
Yeah, homu's been painful the past day or three. Let me poke around. |
Contributor
|
LGTM, thanks a bunch @edsantiago, excellent work as usual. |
Contributor
|
@rh-atomic-bot retry |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 30, 2019
Well, yikes: fix a broken file-mode check in two 'add' tests. They were expecting 755, but the actual mode is 644. --debug was spitting out unrelated output, including a 755 that the test was matching as a substring. Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: #1935 Approved by: giuseppe
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 30, 2019
- add explicit --log-level=debug in some buildah-bud invocations where we need to check debug output - use expect_line_count() in one place, to get better diagnostics - formats test: refactor, make more readable - imgtype invoker: remove -debug - fix one more instance of 'step' needing to be 'STEP' - fix one mkdir to mkdir -p, and run rm -rf beforehand, to deal with a failed prior run that might have left state Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: #1935 Approved by: giuseppe
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 30, 2019
The newer-than and older-than test was failing (desired result) but it's because the command had a syntax error (not desired)! Rewrite that section a little, making it more robust (actually confirming that mtime is preserved, versus simple mtime match because of time of execution) and also with more readable diagnostic in case of error. Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: #1935 Approved by: giuseppe
|
☀️ Test successful - status-papr, status-travis |
caiges
pushed a commit
to caiges/buildah
that referenced
this pull request
Nov 12, 2019
First, and possibly most controversial, remove --debug flag. Output on test failure is painful to read. It is unlikely (but not inconceivable) that --debug will be of any use. Second, fix a failure message so it properly uses %s instead of string interpolation. (If the test name itself includes a percent sign, we get undefined behavior on output). Fix a few instances of 'buildah' to be 'run_buildah'. Remove some unnecessary 'run/echo/check-status' sequences, sticking with the much cleaner 'test'. When BATS shows an error of the form 'this failed: [ $status -eq 0 ]' I weep in despair. Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: podman-container-tools#1935 Approved by: giuseppe
caiges
pushed a commit
to caiges/buildah
that referenced
this pull request
Nov 12, 2019
Well, yikes: fix a broken file-mode check in two 'add' tests. They were expecting 755, but the actual mode is 644. --debug was spitting out unrelated output, including a 755 that the test was matching as a substring. Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: podman-container-tools#1935 Approved by: giuseppe
caiges
pushed a commit
to caiges/buildah
that referenced
this pull request
Nov 12, 2019
- add explicit --log-level=debug in some buildah-bud invocations where we need to check debug output - use expect_line_count() in one place, to get better diagnostics - formats test: refactor, make more readable - imgtype invoker: remove -debug - fix one more instance of 'step' needing to be 'STEP' - fix one mkdir to mkdir -p, and run rm -rf beforehand, to deal with a failed prior run that might have left state Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: podman-container-tools#1935 Approved by: giuseppe
caiges
pushed a commit
to caiges/buildah
that referenced
this pull request
Nov 12, 2019
The newer-than and older-than test was failing (desired result) but it's because the command had a syntax error (not desired)! Rewrite that section a little, making it more robust (actually confirming that mtime is preserved, versus simple mtime match because of time of execution) and also with more readable diagnostic in case of error. Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: podman-container-tools#1935 Approved by: giuseppe
edsantiago
added a commit
to edsantiago/buildah
that referenced
this pull request
Dec 16, 2019
PR podman-container-tools#1935 removed the default --debug logging; so all run_buildah calls now use the default (error). It is safe to remove unnecessary instances of --log-level=error . For ease of review, this commit was entirely machine-generated via: $ perl -pi -e 's/ --log-level=error / /' *.bats Signed-off-by: Ed Santiago <santiago@redhat.com>
rh-atomic-bot
pushed a commit
that referenced
this pull request
Dec 17, 2019
PR #1935 removed the default --debug logging; so all run_buildah calls now use the default (error). It is safe to remove unnecessary instances of --log-level=error . For ease of review, this commit was entirely machine-generated via: $ perl -pi -e 's/ --log-level=error / /' *.bats Signed-off-by: Ed Santiago <santiago@redhat.com> Closes: #2029 Approved by: rhatdan
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First, and possibly most controversial, remove --debug flag.
Output on test failure is painful to read. It is unlikely
(but not inconceivable) that --debug will be of any use.
Second, fix a failure message so it properly uses %s instead
of string interpolation. (If the test name itself includes
a percent sign, we get undefined behavior on output).
Fix a few instances of 'buildah' to be 'run_buildah'.
Remove some unnecessary 'run/echo/check-status' sequences,
sticking with the much cleaner 'test'. When BATS shows
an error of the form 'this failed: [ $status -eq 0 ]'
I weep in despair.
Signed-off-by: Ed Santiago santiago@redhat.com