Skip to content

Testsuite driven py3 #17

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

Closed
wants to merge 21 commits into from
Closed

Conversation

ydirson
Copy link
Contributor

@ydirson ydirson commented Sep 21, 2022

Last commits for full python3 compatibility

@ydirson ydirson force-pushed the testsuite-driven-py3 branch 2 times, most recently from 25326e7 to ec9d4ea Compare September 26, 2022 08:46
@ydirson ydirson marked this pull request as draft September 26, 2022 08:47
@ydirson
Copy link
Contributor Author

ydirson commented Sep 26, 2022

Commit "fix xmlunwrap and its test to align with the use of bytes" raises a question, whether we don't want, rather, to fix the xcp.xmlunwrap API to use str and not bytes.

@ydirson ydirson force-pushed the testsuite-driven-py3 branch from ec9d4ea to 92b2913 Compare September 26, 2022 11:47
@ydirson
Copy link
Contributor Author

ydirson commented Sep 26, 2022

Commit "switch from ConfigParser to configparser" raises a problem with the Accessor API, which does not allow to request text or binary streams, which allowed different implementations to return one or the other. Commit "test_accessor: check for I/O on binary files" then proceeds to demonstrate the problem, in the way it will impact host-upgrade-plugin.

My opinion is, we should change the Accessor API to add a mode parameter to openAddress(). Making it mandatory will make sure we don't miss any client code. @edwintorok what do you think ?

Too bad by parametrization of the tests make it harder to mark just the single failing case as expectedFailure, but I expect that one is not a problem with you ?

@ydirson ydirson force-pushed the testsuite-driven-py3 branch from 92b2913 to c41e019 Compare September 26, 2022 12:19
@edwintorok
Copy link
Contributor

Adding the mode parameter seems like the right way forward but it will tie merging and pushing this PR together with other PRs fixing up all users, in that case it might be good to leave that for a 2nd PR so we can merge part of the 2-to-3 conversion without requiring other packages to move or upgrade just yet.

@ydirson ydirson mentioned this pull request Sep 26, 2022
@ydirson
Copy link
Contributor Author

ydirson commented Sep 26, 2022

Adding the mode parameter seems like the right way forward but it will tie merging and pushing this PR together with other PRs fixing up all users, in that case it might be good to leave that for a 2nd PR so we can merge part of the 2-to-3 conversion without requiring other packages to move or upgrade just yet.

Yes we could have a separate PR, but this is still part of the 2-to-3 conversion, and requires to push "switch from ConfigParser to configparser" as is, including the workaround for the issue - or just acknowledge that the 2-to-3 work is known not to be complete at this stage and not include the hackish commit, waiting for API refactor.

Similarly for the xmlunwrap API issue, which also revolves around str vs. bytes.

ydirson and others added 12 commits January 20, 2023 17:45
Use of `unicode` needed to be immediately handled, but a few checks
relying on `str` could become insufficient in python2 with the larger
usage of unicode strings.

Signed-off-by: Yann Dirson <[email protected]>
…s to

open() as ths is considered best practice.

(cherry picked from cpython commit 6cef076ba5edbfa42239924951d8acbb087b3b19)

Signed-off-by: Yann Dirson <[email protected]>
Running tests on python3 did reveal some of them.

Signed-off-by: Yann Dirson <[email protected]>
There is no guaranty about ordering of dict elements, and tests compare
results derived from enumerating a dict element.  We could have used an
OrderedDict to store the formulae and get a predictible output order, but
just considering the output as a set seems better.

Only applying this to rules expected to hold more than one element.

Signed-off-by: Yann Dirson <[email protected]>
Caught by extended test.

Signed-off-by: Yann Dirson <[email protected]>
FIXME: I'm quite unsure why xcp.xmlunwrap would want to use bytes and not
unicode strings, but the encode/decode calls make it quite clear it wants
to work with bytes.  That makes the API painful to use in python3.
hashlib came with python 2.5, and old md5 module disappears in 3.0

Signed-off-by: Yann Dirson <[email protected]>
This is supposed to be just a module renaming to conform to PEP8, see
https://docs.python.org/3/whatsnew/3.0.html#library-changes

The SafeConfigParser class has been renamed to ConfigParser in Python
3.2, and backported as addon package.  The `readfp` method now
triggers a deprecation warning to replace it with `read_file`.

FIXME: With python3 some Accessor implementations (e.g. FileAccessor)
provide a text stream for repository config (and with python2 all
implementations), while others (e.g. HTTPAccessor) provide a binary
stream.  But on python3 ConfigParser will bomb out if given a binary
stream, so use a TextIOWrapper to access the config.  This is a hack,
which cannot be used when it is binary data which has to be read (see
later commits), so I don't consider this commit to be correct in that
respect.
Testing several accessor classes causes code duplication, which can be
avoided with help from the `parametrized` package (unfortunately, `pytest`
support cannot be used together with `unittest`).

Not a big deal right now, but starts becoming painful when adding new tests
or testing other Accessor classes.

Signed-off-by: Yann Dirson <[email protected]>
This test uses the same kind of I/O (file copy) that prepare_host_upgrade.py
does.

FIXME: the copy cannot proceed this way in python3
This works properly for the http case, but FileAccessor provides us with
a text fileobj handle, and `read()` gets a UTF-8 decoding error.

FIXME: Accessor ctor requires a `mode` argument
Reported under python3 for members created on-the-fly in `setUp()`

Signed-off-by: Yann Dirson <[email protected]>
With python3, pylint complains about `else: raise()` constructs.
This rework avoids them and reduces cyclomatic complexity by using
the error-out-first idiom.

Signed-off-by: Yann Dirson <[email protected]>
diff-cover defaults to origin/main in new version, it seems.

Signed-off-by: Yann Dirson <[email protected]>
@bernhardkaindl
Copy link
Collaborator

This PR has been used as the basis of #27 which as been merged to master now, closing as done!

bernhardkaindl added a commit to rosslagerwall/python-libs that referenced this pull request May 8, 2024
…/CP-41819-pip-pyproject-isort

CP-41819: Start py3 migration with config for isort and apply isort
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.

4 participants