Skip to content
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

mimic: cephfs: test_volume_client: fix test_put_object_versioned() #30236

Merged
merged 6 commits into from Oct 10, 2019

Conversation

@smithfarm
Copy link
Contributor

smithfarm commented Sep 7, 2019

Convert strings to bytes in the Python program embedded inside this
test. Also, in the process don't lose Python 2 compatibility.

Fixes: http://tracker.ceph.com/issues/39405
Signed-off-by: Rishabh Dave <ridave@redhat.com>
(cherry picked from commit 975b475)
Instead of defining another method to write a file as root user in
test_volume_client.py, use the method from teuthology/misc.py.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
(cherry picked from commit 878c34c)
The test's success depends on whether CommandFailedError exception is
raised. This manner of testing is unreliable since there's no way to
check if the exception was raised by the statement (in the embedded
Python program) the test expects to.

This implies that this test's failure may go undetected. This is primary
reason why bugs like tracker #38946 occured despite of having a test.

Fixes: http://tracker.ceph.com/issues/39510
Signed-off-by: Rishabh Dave <ridave@redhat.com>
(cherry picked from commit 37744ec)
test_put_object_versioned() is too generic for a negative test that
specifically tests if the version is verified before writing.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
(cherry picked from commit 7980e1f)
Name class and instance variables differently so that the latter doesn't
shadow the former.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
(cherry picked from commit 4870574)
ceph_volume_client.py's put_object_versioned() has only one test. Since
this only test is a negative test it may fail to assure that
put_object_versioned() works as expected with positive inputs even when
it completes successfully. Therefore, write a positive test for better
coverage.

Also, make sure the new test is both python 2 and python3 compatible.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
(cherry picked from commit 9aa9ff0)
@smithfarm smithfarm added this to the mimic milestone Sep 7, 2019
@smithfarm smithfarm added the cephfs label Sep 7, 2019
@smithfarm smithfarm requested review from rishabh-d-dave and batrick Sep 7, 2019
@smithfarm smithfarm changed the title mimic: test_volume_client: fix test_put_object_versioned() mimic: cephfs: test_volume_client: fix test_put_object_versioned() Sep 7, 2019
Copy link
Contributor

rishabh-d-dave left a comment

Otherwise, look fine to me.

PS: My comments aren't relevant if we would never make tests python3 compliant.
@batrick We won't make our tests python3 compliant in older releases, right? I'll mark this PR approved in that case.

if sys_version_info.major < 3:
data = data + 'modification1'
elif sys_version_info.major > 3:

This comment has been minimized.

Copy link
@rishabh-d-dave

rishabh-d-dave Sep 11, 2019

Contributor

should be >= 3 instead of > 3.

if sys_version_info.major < 3:
data = data + 'm1'
elif sys_version_info.major > 3:

This comment has been minimized.

Copy link
@rishabh-d-dave

rishabh-d-dave Sep 11, 2019

Contributor

same here

if sys_version_info.major < 3:
data = data + 'm2'
elif sys_version_info.major > 3:

This comment has been minimized.

Copy link
@rishabh-d-dave

rishabh-d-dave Sep 11, 2019

Contributor

and here

@batrick

This comment has been minimized.

Copy link
Member

batrick commented Sep 11, 2019

@batrick We won't make our tests python3 compliant in older releases, right? I'll mark this PR approved in that case.

Tests are always run as python2 until we someday (if ever) update teuthology to py3.

In any case, backports are usually not the right place to make these compatibility fixes. They should have been addressed in the original PR.

@rishabh-d-dave

This comment has been minimized.

Copy link
Contributor

rishabh-d-dave commented Sep 11, 2019

@batrick We won't make our tests python3 compliant in older releases, right? I'll mark this PR approved in that case.

Tests are always run as python2 until we someday (if ever) update teuthology to py3.

In any case, backports are usually not the right place to make these compatibility fixes. They should have been addressed in the original PR.

Ok. I'll create a new PR for that and approve this one.

@yuriw

This comment has been minimized.

Copy link
Contributor

yuriw commented Oct 7, 2019

@yuriw yuriw merged commit 5bcdd91 into ceph:mimic Oct 10, 2019
4 checks passed
4 checks passed
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

smithfarm commented Oct 11, 2019

@rishabh-d-dave Teuthology, the "make check CI", and other testing infrastructure (Jenkins) is not maintained in separate stable branches like ceph is. For this reason, we cannot assume that older branches of ceph will continue to be tested using the same Python 2 testing infrastructure that was in use at the time the release was first published. Teuthology is in the process of being migrated to Python 3, and all the Python 2 testing code should be made compatible with Python 3 in all the branches (not just the newer ones).

@smithfarm smithfarm deleted the smithfarm:wip-40853-mimic branch Oct 11, 2019
@batrick

This comment has been minimized.

Copy link
Member

batrick commented Oct 11, 2019

and all the Python 2 testing code should be made compatible with Python 3 in all the branches (not just the newer ones).

Compatibility is tricky here @smithfarm. I think we should endeavor to write code which is forwards and backwards compatible but we should avoid any code like:

if python3:
  do X
else:
  do Y

The python3 code is not actually going to be tested because no one is running the code with py3. Better to just leave it incompatible until we are ready to switch IMO. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.