Add some tests and test steps for buildah #1966

ypu · 2019-11-06T11:41:36Z

Add several tests and also add some check points for exist test cases in buildah/tests

rh-atomic-bot · 2019-11-06T11:42:47Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

TomSweeneyRedHat · 2019-11-06T13:53:09Z

tests/add.bats

+
+ expect_output --substring bin.*bin
+ buildah ps
+ buildah images


just wondering why you're doing the ps and images commands without testing them? I'd say drop them, but if you think they're important, then change buildah in each to run_buildah.

I know I'm guilty of this too, but are the rm and rmi call necessary with the current test harness @edsantiago

Sorry this is for some debug information during the tests. Will remove them.

TomSweeneyRedHat · 2019-11-06T13:54:21Z

tests/from.bats

@@ -345,3 +346,24 @@ load helpers
 run_buildah --log-level=error containers -f id=${cid}
 buildah rm ${cid}
 }
+
+@test "from with/without pull test" {
+ run buildah from --pull=false docker.io/nginx


s/run_buildah/run buildah/ through out.

Actually s/run buildah/run_buildah/ but yes, what he said.

TomSweeneyRedHat · 2019-11-06T13:56:14Z

tests/push.bats

+ for dest in docker-archive oci-archive; do
+ mkdir ${TESTDIR}/tmp
+ run_buildah push --signature-policy ${TESTSDIR}/policy.json busybox $dest:${TESTDIR}/tmp/busybox.tar:latest
+ run ls ${TESTDIR}/tmp/busybox.tar


I don't think you need the 'run' here

Thanks for reminding. status check can be covered run the command directly. Will update it.

TomSweeneyRedHat · 2019-11-06T13:56:28Z

tests/push.bats

+
+ buildah pull --signature-policy ${TESTSDIR}/policy.json busybox
+ run_buildah push --signature-policy ${TESTSDIR}/policy.json busybox docker-daemon:buildah/busybox:latest
+ run docker images


This run is used for update the $output context for the following expect_output check. So will keep it.

TomSweeneyRedHat · 2019-11-06T13:57:06Z

@edsantiago if you some time to eyeball this, I'd appreciate it.

edsantiago

Enough. Please fix the above, and please also extrapolate and fix all the rest of the same instances of the same problems (run_buildah, output checks, rmis, etc etc). I'll re-review the new submission. Thanks.

tests/add.bats

edsantiago · 2019-11-06T15:13:58Z

tests/add.bats

 run_buildah rm $newcid
 }
+
+@test "add with chown" {


Is this really necessary as a separate test? Each separate test adds many seconds to the full run, and more if it has to pull an image. Why not just incorporate the --chown into the previous test? (line 131 above), and change the stat and expect in lines 132-134 to:

run_buildah run $cid stat -c "%U:%G:%a" lib/custom expect_output "bin:bin:$permission" "ownership/mode of file copied into container"

(side note: the buildah mount in line 133 seems to be completely unnecessary. I think it's a copy-paste remnant. Feel free to remove it)

Hi @edsantiago
I checked that case but it is not a test for --chown flag but a basic tests for check buildah add file permissions. I'd like to keep per test cases to test a single function, so QE can find the problem quickly, and another reason I think we should keep a test case simple is the test case will be interrupted if one of the step failed and following steps will be cut off from the test results.

I don't think I communicated my point well enough: I was trying to say that this test is almost identical to the previous one, and it would make sense to combine the two. Better readability and maintainability. But I won't block because of that.

edsantiago · 2019-11-06T15:16:08Z

tests/add.bats

+ run_buildah run $cid ls /home/README.md
+
+ buildah rm $cid
+ buildah rmi busybox


Why the rmi? It seems wasteful to force other tests to re-pull the image. It's also a great way to increase test runtime and increase risk of flakes due to network failures. I don't think we want that.

OK. Will remove it. Just want to keep the test env clean and not influence other test cases. But yes, this is not necessary for every test. Will check them and remove the ones not needed. Thanks a lot.

edsantiago · 2019-11-06T15:17:08Z

tests/bud.bats

@@ -1821,3 +1821,11 @@ load helpers

 rm -f ${TESTSDIR}/bud/${target}/test*
 }
+
+@test "bud quite" {


Typo: I think you mean "quiet". Please fix in lines 1825-1829.

Get it. Thanks a lot.

edsantiago · 2019-11-06T15:18:17Z

tests/bud.bats

+
+@test "bud quite" {
+ run_buildah bud --format docker -t quite-test --signature-policy ${TESTSDIR}/policy.json -q ${TESTSDIR}/bud/shell
+ expect_line_count 1


What if the line is a warning, or something unexpected? Please also include an expect_output.

edsantiago · 2019-11-06T15:39:39Z

tests/from.bats

+ run buildah from --name nginxc docker.io/nginx
+ expect_output --substring "Getting"
+ run buildah from --name nginxc-1 docker.io/nginx
+ [[ "$output" != "Getting"* ]]


See above comment re: this not doing what you think.

I test it in my fedora31 and it works as expect.
Here is the code:
run buildah pull busybox echo $output [[ "$output" != "Getting"* ]]
And the restuls:

# '[[ "$output" != "Getting"* ]]' failed with status 127 # # Getting image source signatures Copying blob sha256:0f8c40e1270f10d085dda8ce12b7c5b17cd808f055df5a7222f54837ca0feae0 Copying config sha256:020584afccce44678ec82676db80f68d50ea5c766b6e9d9601f7b5fc86dfb96d Writing manifest to image destination Storing signatures 020584afccce44678ec82676db80f68d50ea5c766b6e9d9601f7b5fc86dfb96d

Yes, it works, but it is not a good test: it does not check what you think it checks. You want to fail if "Getting" appears anywhere in the output; this test only fails if the output begins with "Getting". Which in most cases it will, but not if there are warning messages or any other output. Please see my comments a few minutes ago about testing string output.

edsantiago · 2019-11-06T15:40:09Z

tests/from.bats

+ [[ "$output" != "Getting"* ]]
+ run buildah from --name nginxc-2 --pull-always docker.io/nginx
+ expect_output --substring "Getting"
+ buildah commit nginxc fakename-img


run_buildah

edsantiago · 2019-11-06T15:40:28Z

tests/from.bats

+ expect_output --substring "Getting"
+ buildah commit nginxc fakename-img
+ run buildah buildah from --pull-always fakename-img
+ [ $status -ne 0 ]


Remove, see above exit-status form of run_buildah comment.

edsantiago · 2019-11-06T15:43:00Z

tests/from.bats

+
+@test "from quiet test" {
+ run buildah from --quiet docker.io/busybox
+ [ "$output" != "Getting"* ]


This is not a valid test, because (1) this-does-not-do-what-you-think-it-does, and (2) you don't know if busybox is already cached locally, in which case there would be no Getting message. A proper test would ensure that the desired image is not already present. This might be an instance in which rmi is necessary, much as I dislike that.

Actually I should use [[ ]] here, and it should works. Will update it. And yes, when I right a test case I default think the env is clean, so the image check is not request here. But as we want to keep the common use images during all tests, I will add the steps for exist images for it. Thanks for your comments.

edsantiago · 2019-11-06T15:43:18Z

tests/images.bats

@@ -157,3 +157,9 @@ load helpers
 buildah rm -a
 buildah rmi -a -f
 }
+
+@test "image digest test" {
+ buildah pull busybox


run_buildah

This is just a prepare step. If I understand the difference between run_buildah and buildah, we should use run_buildah in the test steps, but use buildah in the prepare steps.

One much more important difference is that run_buildah will display the command being run, and its output, and will check its exit status. If buildah pull fails (probably because of a network flake), BATS will exit with a hard-to-debug error. run_buildah will give much better information.

rh-atomic-bot · 2019-11-06T22:20:54Z

☔ The latest upstream changes (presumably 89bc2a6) made this pull request unmergeable. Please resolve the merge conflicts.

Add following tests for buildah add 1. buildah add test with --chown 2. buildah add url test Signed-off-by: Yiqiao Pu <[email protected]>

Signed-off-by: Yiqiao Pu <[email protected]>

TomSweeneyRedHat · 2019-11-08T15:08:22Z

@ypu it looks like you've been doing a lot of great work addressing comments in this PR, TY! Once you're complete and have pushed the last bits, can you please add a reply to this PR that it's ready for review again? Thanks!

Add following test cases for commit: - commit with name - commit to docker-distribution Signed-off-by: Yiqiao Pu <[email protected]>

Signed-off-by: Yiqiao Pu <[email protected]>

Add following test cases for push: - buildah push image to containers-storage - buildah push image to docker-archive and oci-archive - buildah push image to docker and docker registry Signed-off-by: Yiqiao Pu <[email protected]>

Signed-off-by: Yiqiao Pu <[email protected]>

The permission may different in different system. Not always a fix value. So update the scripts to get the permission during the test. Signed-off-by: Yiqiao Pu <[email protected]>

Add some checkpoint for checking container name and id inside the tests. Signed-off-by: Yiqiao Pu <[email protected]>

Add check steps for inspect with container id and image id. Signed-off-by: Yiqiao Pu <[email protected]>

ypu · 2019-11-11T01:37:43Z

Hi @TomSweeneyRedHat and @edsantiago Thanks a lot for your review and comments. Now I update the code to date to fit the travis-ci env. Can you help to review them again? Thanks again.

TomSweeneyRedHat · 2019-11-11T13:28:21Z

@edsantiago still had a few outstanding questions that you answered, but he's not replied to. Other than that, this LGTM. FYI, @edsantiago will soon be traveling, so might not get to this for a bit.

rh-atomic-bot · 2019-11-16T20:52:17Z

☔ The latest upstream changes (presumably 9ff68b3) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2019-11-20T14:38:27Z

@ypu Please rebase and answer @edsantiago questions. Would love to get this in.

edsantiago · 2019-12-02T14:28:59Z

Ping, this is good work, it only needs some fixes and a rebase

rhatdan · 2019-12-02T16:57:38Z

@edsantiago could you take this over and push it over the finish line?

edsantiago · 2019-12-02T17:04:41Z

@rhatdan let's give @ypu an overnight (for us) chance to respond; I'll get on this tomorrow if nak.

PR 1966 has languished for three weeks without activity from submitter. In the interests of getting it online, I have taken it over and: - rebased - fixed several misunderstandings (bugs) noted in review feedback - fixed a few more I also slightly rewrote two tests (tag by id, commit with name) that were incomprehensible to me: unnecessary mount/umount and no actual testing of anything other than checking exit status. I believe the new code is closer to the intention of testing but please pay closer attention to those bits. Also: fixed the basic 'inspect' test. It looks like at some point in the last month containers#1917 added a version string to the buildah-inspect output. The test was fixed on master, but ypu's PR did not incorporate those fixes and the test was breaking. I took the liberty of cleaning up the entire test for readability and maintainability. Signed-off-by: Ed Santiago <[email protected]>

PR 1966 has languished for three weeks without activity from submitter. In the interests of getting it online, I have taken it over and: - rebased - fixed several misunderstandings (bugs) noted in review feedback - fixed a few more I also slightly rewrote two tests (tag by id, commit with name) that were incomprehensible to me: unnecessary mount/umount and no actual testing of anything other than checking exit status. I believe the new code is closer to the intention of testing but please pay closer attention to those bits. Also: fixed the basic 'inspect' test. It looks like at some point in the last month #1917 added a version string to the buildah-inspect output. The test was fixed on master, but ypu's PR did not incorporate those fixes and the test was breaking. I took the liberty of cleaning up the entire test for readability and maintainability. Signed-off-by: Ed Santiago <[email protected]> Closes: #2004 Approved by: rhatdan

rhatdan · 2019-12-06T12:02:28Z

Since @edsantiago finished this PR CLosing.

ypu force-pushed the tests branch from d0a83e3 to ffffc4e Compare November 6, 2019 13:20

TomSweeneyRedHat reviewed Nov 6, 2019

View reviewed changes

edsantiago requested changes Nov 6, 2019

View reviewed changes

ypu added 2 commits November 7, 2019 17:33

Tests: Add two test for buildah add

8d7eb39

Add following tests for buildah add 1. buildah add test with --chown 2. buildah add url test Signed-off-by: Yiqiao Pu <[email protected]>

Tests: Add buildah bud with --quiet test

7906b01

Signed-off-by: Yiqiao Pu <[email protected]>

ypu force-pushed the tests branch 4 times, most recently from 13be7c1 to a29904e Compare November 8, 2019 14:05

ypu added 8 commits November 9, 2019 21:32

Tests: Add two commit test

38337d8

Add following test cases for commit: - commit with name - commit to docker-distribution Signed-off-by: Yiqiao Pu <[email protected]>

Tests: Add some buildah from tests

8c9d2f9

Signed-off-by: Yiqiao Pu <[email protected]>

Tests: Add image digest test

a0f50a8

Signed-off-by: Yiqiao Pu <[email protected]>

Tests: Add test cases for push test

6a6a365

Add following test cases for push: - buildah push image to containers-storage - buildah push image to docker-archive and oci-archive - buildah push image to docker and docker registry Signed-off-by: Yiqiao Pu <[email protected]>

Tests: Add a test for tag by id

92c2652

Signed-off-by: Yiqiao Pu <[email protected]>

Test: Get permission in add test

fbce1c3

The permission may different in different system. Not always a fix value. So update the scripts to get the permission during the test. Signed-off-by: Yiqiao Pu <[email protected]>

Tests: Add container name and id check in containers test steps

5e8fc71

Add some checkpoint for checking container name and id inside the tests. Signed-off-by: Yiqiao Pu <[email protected]>

Tests: Add inpsect test check steps

756d6eb

Add check steps for inspect with container id and image id. Signed-off-by: Yiqiao Pu <[email protected]>

ypu force-pushed the tests branch from a29904e to 756d6eb Compare November 9, 2019 13:52

edsantiago mentioned this pull request Dec 3, 2019

Ypu bats tests #2004

Closed

rhatdan closed this Dec 6, 2019

github-actions bot added the locked - please file new issue/PR label Oct 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2023

Add some tests and test steps for buildah #1966

Add some tests and test steps for buildah #1966

Conversation

ypu commented Nov 6, 2019

rh-atomic-bot commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Nov 6, 2019

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypu Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rh-atomic-bot commented Nov 6, 2019

TomSweeneyRedHat commented Nov 8, 2019

ypu commented Nov 11, 2019

TomSweeneyRedHat commented Nov 11, 2019

rh-atomic-bot commented Nov 16, 2019

rhatdan commented Nov 20, 2019

edsantiago commented Dec 2, 2019

rhatdan commented Dec 2, 2019

edsantiago commented Dec 2, 2019

rhatdan commented Dec 6, 2019

ypu Nov 7, 2019 •

edited

Loading