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

cc_mounts: fix incorrect format specifiers #316

Merged
merged 1 commit into from
Apr 15, 2020

Conversation

OddBloke
Copy link
Collaborator

LP: #1872836

@mock.patch(M_PATH + 'util.subp')
def test_happy_path(self, m_subp, tmpdir):
swap_file = tmpdir.join("swap-file")
fname = str(swap_file)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this just swap_file.strpath?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had to use swap_file.strpath in ubuntu-advantage-tools because str(swap_file) didn't work in the version of pytest in trusty. However, using str() is preferable because it works against more types, so if swap_file changes type for whatever reason then this line doesn't need changing.

(And there is a slight practical component to this: more recent versions of pytest have introduced the tmp_path fixture which returns an instance of the stdlib's pathlib.Path instead of the third-party-and-in-maintenance-mode py library's py.path.LocalPath. str(tmp_path.joinpath(...)) behaves the same as str(tmpdir.join(...)), but tmp_path.joinpath(...).strpath raises an AttributeError. Eventually (after we're no longer backporting to bionic, now that I look at the version tmp_path was introduced in), we'll want to migrate to tmp_path.)

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit on the unit test and we can skip the unhappy_path test given the timeliness of the release and it basically exercises the same type of logic fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants