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

Test fixes: unit.fileserver.test_gitfs #49244

Merged
merged 4 commits into from Aug 22, 2018

Conversation

Projects
None yet
3 participants
@dwoz
Contributor

dwoz commented Aug 22, 2018

What does this PR do?

Multiple fixes for unit.fileserver.test_gitfs:

  • file:// source repos need all forward slashes
  • Use salt.utils.files.rm_rf for recursive removes
  • Patch salt.utils.files.rm_rf to work with unicode filenames on windows python 2

Fixes:

unit.fileserver.test_gitfs.GitPythonTest.test_dir_list
unit.fileserver.test_gitfs.GitPythonTest.test_disable_saltenv_mapping_global_with_mapping_defined_globally
unit.fileserver.test_gitfs.GitPythonTest.test_disable_saltenv_mapping_global_with_mapping_defined_per_remote
unit.fileserver.test_gitfs.GitPythonTest.test_disable_saltenv_mapping_per_remote_with_mapping_defined_globally
unit.fileserver.test_gitfs.GitPythonTest.test_disable_saltenv_mapping_per_remote_with_mapping_defined_per_remote
unit.fileserver.test_gitfs.GitPythonTest.test_envs
unit.fileserver.test_gitfs.GitPythonTest.test_file_list
unit.fileserver.test_gitfs.GitPythonTest.test_ref_types_global
unit.fileserver.test_gitfs.GitPythonTest.test_ref_types_per_remote

https://jenkinsci.saltstack.com/job/2018.3/job/salt-windows-2016-py2/28/#showFailuresLink

Tests written?

Yes - fixing tests

Commits signed with GPG?

Yes

dwoz added some commits Aug 22, 2018

Remove unicode filenames on windows python 2
Need to provide unicode to shutil in order to retrieve and delete
unicode.

@dwoz dwoz requested a review from terminalmage Aug 22, 2018

@dwoz dwoz force-pushed the dwoz:gitfs_fixes branch from 6084bb3 to 5a0cda5 Aug 22, 2018

@@ -547,6 +547,8 @@ def _onerror(func, path, exc_info):
if os.path.islink(path) or not os.path.isdir(path):
os.remove(path)
else:
if six.PY2 and salt.utils.platform.is_windows() and isinstance(path, six.string_types):
path = path.decode('utf-8')

This comment has been minimized.

@terminalmage

terminalmage Aug 22, 2018

Member

salt.utils.stringutils.to_unicode() should be used here instead. The reason for this is that the unicode type's decode() method will attempt to encode the string before decoding, because reasons:

>>> u'Д'.decode('utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/encodings/utf_8.py", line 16, in decode
    return codecs.utf_8_decode(input, errors, True)
UnicodeEncodeError: 'ascii' codec can't encode character u'\u0414' in position 0: ordinal not in range(128)

We can skip the PY2 check completely because the to_unicode() helper already performs this check. So, these two lines can be alternately written as:

if salt.utils.platform.is_windows():
    try:
        path = salt.utils.stringutils.to_unicode(path)
    except TypeError:
        pass

If to_unicode() is passed a non string/bytestring/bytearray type, it will raise a TypeError, hence the exception catching. This also allows us to avoid the isinstance check.

@@ -422,6 +424,9 @@ def setUpClass(cls):
# Add a tag
repo.create_tag(TAG_NAME, 'HEAD')
# Older GitPython versions do not have a close method.
if hasattr(repo, 'close'):
repo.close()

This comment has been minimized.

@terminalmage

terminalmage Aug 22, 2018

Member

Good catch 👍

@dwoz dwoz merged commit ec32428 into saltstack:2018.3 Aug 22, 2018

3 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
jenkins/pr/py2-centos-7 running py2-centos-7...
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details

rallytime added a commit that referenced this pull request Aug 28, 2018

@dwoz dwoz deleted the dwoz:gitfs_fixes branch Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment