-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix ImageCollectionTest.test_pull_multiple flakiness #2485
Fix ImageCollectionTest.test_pull_multiple flakiness #2485
Conversation
ping @ulyssessouza @shin- ptal |
71c20ba
to
9f65c2b
Compare
Ah booh. It's failing. Not sure what I did wrong 🤔
|
assert len(images) >= 1 | ||
found = False | ||
for img in images: | ||
if 'hello-world:latest' == img.attrs['RepoTags']: |
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.
Hi @thaJeztah
Could you try to use Unicode for image name, like u'hello-world:latest'
?
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.
Sure, I can try that (would that be a bit odd though, given that image references only support a-z0-9 ? 🤔 or does python return it as unicode? (I'm not really a Python expert 😅)
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.
Python 2 doesn't use Unicode as default.
Also, I forgot that img.attrs['RepoTags']
is a list, so you can change the comparator ==
to in
and the test will pass.
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.
Doh! You're right, completely forgot about that; yes, that should be the problem
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.
Yeah, Unicode wasn't the bad guy this time. hahaha
b7c4d7a
to
39c1fbb
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
all green now! thanks for your help (and review) @feliperuhland 🤗 |
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
assert 'hello-world:latest' in images[0].attrs['RepoTags'] | ||
assert len(images) >= 1 | ||
found = False | ||
for img in images: |
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.
Could you use any()
instead of doing a for loop. E.g. something like assert any('hello-world:latest' in img[0].attrs['RepoTags'] for img in images)
?
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.
Ah, yes, sure. Didn't know of that approach 🤗
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.
It's just img.attrs
, not img[0].attrs
.
Edit: the code looks better.
39c1fbb
to
9d5d963
Compare
The ImageCollectionTest.test_pull_multiple test performs a `docker pull` without a `:tag` specified) to pull all tags of the given repository (image). After pulling the image, the image(s) pulled are checked to verify if the list of images contains the `:latest` tag. However, the test assumes that all tags of the image are tags for the same version of the image (same digest), and thus a *single* image is returned, which is not always the case. Currently, the `hello-world:latest` and `hello-world:linux` tags point to a different digest, therefore the `client.images.pull()` returns multiple images: one image for digest, making the test fail: =================================== FAILURES =================================== ____________________ ImageCollectionTest.test_pull_multiple ____________________ tests/integration/models_images_test.py:90: in test_pull_multiple assert len(images) == 1 E AssertionError: assert 2 == 1 E + where 2 = len([<Image: 'hello-world:linux'>, <Image: 'hello-world:latest'>]) This patch updates the test to not assume a single image is returned, and instead loop through the list of images and check if any of the images contains the `:latest` tag. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
9d5d963
to
940805d
Compare
@zappy-shu @ulyssessouza @feliperuhland updated; ptal |
@thaJeztah looks great |
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
relates to moby/moby#40340 (moby/moby#40340 (comment))
relates to moby/moby#40343
introduced in #1883
The ImageCollectionTest.test_pull_multiple test performs a
docker pull
withouta
:tag
specified) to pull all tags of the given repository (image).After pulling the image, the image(s) pulled are checked to verify if the list
of images contains the
:latest
tag.However, the test assumes that all tags of the image are tags for the same
version of the image (same digest), and thus a single image is returned, which
is not always the case.
Currently, the
hello-world:latest
andhello-world:linux
tags point to adifferent digest, therefore the
client.images.pull()
returns multiple images:one image for digest, making the test fail:
This patch updates the test to not assume a single image is returned, and instead
loop through the list of images and check if any of the images contains the
:latest
tag.