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

Fix unit test on Python 3.8 #56031

Merged
merged 6 commits into from
Apr 24, 2020
Merged

Fix unit test on Python 3.8 #56031

merged 6 commits into from
Apr 24, 2020

Conversation

bdrung
Copy link
Contributor

@bdrung bdrung commented Jan 30, 2020

Salt fails on Python 3.8:

======================================================================
ERROR: unit.grains.test_core (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: unit.grains.test_core
Traceback (most recent call last):
  File "/usr/lib/python3.8/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.8/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "tests/unit/grains/test_core.py", line 37, in <module>
    import salt.grains.core as core
  File "salt/grains/core.py", line 40, in <module>
    from platform import _supported_dists
ImportError: cannot import name '_supported_dists' from 'platform' (/usr/lib/python3.8/platform.py)

So only try to import _supported_dists from platform for Python <=
3.7. Otherwise rely on the external distro module to not need any
special handling.

Addresses parts of #55835

@bdrung bdrung requested a review from a team as a code owner January 30, 2020 17:42
@ghost ghost requested a review from cmcmarrow January 30, 2020 17:42
@bdrung bdrung force-pushed the master-python-3.8 branch 2 times, most recently from fb6cfa7 to cc541a2 Compare February 3, 2020 11:44
@bdrung bdrung changed the title Support distro.linux_distribution Fix unit test on Python 3.8 Feb 3, 2020
@bdrung
Copy link
Contributor Author

bdrung commented Feb 3, 2020

I pushed more commits to fix all Python 3.8 related unit tests failures.

@Akm0d Akm0d added this to the Approved milestone Feb 7, 2020
@Akm0d Akm0d self-assigned this Feb 7, 2020
@s0undt3ch
Copy link
Collaborator

FYI, I also did some work on this here.
I'm not devoting much time to it right now, so feel free to rebase your work on top of that branch, it would be much appreciated.

@Akm0d Akm0d added the Core relates to code central or existential to Salt label Feb 7, 2020
@bdrung
Copy link
Contributor Author

bdrung commented Feb 10, 2020

I picked the commits from your branch that I haven't addressed yet.

s-t-e-v-e-n-k added a commit to s-t-e-v-e-n-k/salt that referenced this pull request Feb 17, 2020
Apply saltstack/salt#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
brejoc pushed a commit to openSUSE/salt that referenced this pull request Feb 20, 2020
Apply saltstack/salt#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
@bdrung bdrung force-pushed the master-python-3.8 branch 3 times, most recently from 3833c85 to bc40225 Compare February 21, 2020 14:36
meaksh pushed a commit to meaksh/salt that referenced this pull request Mar 10, 2020
Apply saltstack#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
meaksh pushed a commit to openSUSE/salt that referenced this pull request Mar 10, 2020
Apply saltstack/salt#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
brejoc pushed a commit to openSUSE/salt that referenced this pull request Mar 20, 2020
Apply saltstack/salt#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
meaksh pushed a commit to openSUSE/salt that referenced this pull request Mar 23, 2020
Apply saltstack/salt#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
meaksh pushed a commit to openSUSE/salt that referenced this pull request Mar 24, 2020
Apply saltstack/salt#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
@sagetherage sagetherage added the ZRelease-Sodium retired label label Mar 24, 2020
Copy link
Contributor

@cmcmarrow cmcmarrow left a comment

Choose a reason for hiding this comment

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

@bdrung thanks for doing this! You’re really close. Just need to get the Windows and Mac test working. Once you have that I think this PR will be good to merge.

@bdrung
Copy link
Contributor Author

bdrung commented Mar 26, 2020

The failing Windows and Mac tests are identical with the failings tests of the master branch. So this merge request does not introduce any regression.

@bdrung
Copy link
Contributor Author

bdrung commented Apr 24, 2020

I rebased this patch set on master to resolve the merge commits. This rebase dropped some commits which are already part of the master branch.

@s0undt3ch
Copy link
Collaborator

It has some lint issues that need fixing now

bdrung and others added 6 commits April 24, 2020 11:27
DeprecationWarning: Using or importing the ABCs from `collections`
instead of from `collections.abc` is deprecated since Python 3.3, and in
3.9 it will stop working.

Therefore try to import the abstract base classes from `collections.abc`
before falling back to `collections`.

Signed-off-by: Benjamin Drung <[email protected]>
Salt fails on Python 3.8:

```
======================================================================
ERROR: unit.grains.test_core (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: unit.grains.test_core
Traceback (most recent call last):
  File "/usr/lib/python3.8/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.8/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "tests/unit/grains/test_core.py", line 37, in <module>
    import salt.grains.core as core
  File "salt/grains/core.py", line 40, in <module>
    from platform import _supported_dists
ImportError: cannot import name '_supported_dists' from 'platform' (/usr/lib/python3.8/platform.py)
```

So only try to import `_supported_dists` from `platform` for Python <=
3.7. Otherwise rely on the external `distro` module to  not need any
special handling.

Addresses parts of saltstack#55835
Signed-off-by: Benjamin Drung <[email protected]>
The following unit tests fail on Python 3.8:

```
======================================================================
ERROR: test_state_config (unit.renderers.test_stateconf.StateConfigRendererTestCase)
[CPU:0.0%|MEM:56.6%]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/<<PKGBUILDDIR>>/tests/unit/renderers/test_stateconf.py", line 74, in test_state_config
    result = self._render_sls('''
  File "/<<PKGBUILDDIR>>/tests/unit/renderers/test_stateconf.py", line 66, in _render_sls
    return self._renderers['stateconf'](
  File "/<<PKGBUILDDIR>>/salt/renderers/stateconf.py", line 227, in render
    for k in six.iterkeys(tmplctx):  # iterate over a copy of keys
RuntimeError: dictionary keys changed during iteration

======================================================================
ERROR: test_apply_cloud_providers_config_extend (unit.test_config.ConfigTestCase)
[CPU:0.0%|MEM:56.6%]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/<<PKGBUILDDIR>>/tests/unit/test_config.py", line 1243, in test_apply_cloud_providers_config_extend
    salt.config.apply_cloud_providers_config(
  File "/<<PKGBUILDDIR>>/salt/config/__init__.py", line 3196, in apply_cloud_providers_config
    for driver, details in six.iteritems(entries):
RuntimeError: dictionary keys changed during iteration

======================================================================
ERROR: test_apply_cloud_providers_config_extend_multiple (unit.test_config.ConfigTestCase)
[CPU:0.0%|MEM:56.6%]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/<<PKGBUILDDIR>>/tests/unit/test_config.py", line 1334, in test_apply_cloud_providers_config_extend_multiple
    self.assertEqual(ret, salt.config.apply_cloud_providers_config(overrides, defaults=DEFAULT))
  File "/<<PKGBUILDDIR>>/salt/config/__init__.py", line 3196, in apply_cloud_providers_config
    for driver, details in six.iteritems(entries):
RuntimeError: dictionary keys changed during iteration

======================================================================
```

Replace the affected for loop of the first case by a dictionary
comprehension to construct the modified dictionary. For the remaining
cases, switch from `iteritems` to `iterkeys`, since the dictionary
values will be modified.

Signed-off-by: Benjamin Drung <[email protected]>
The test case `unit.modules.test_virt.VirtTestCase.test_update` fails on
Ubuntu 20.04 with Python 3.8:

```
======================================================================
FAIL: test_update (unit.modules.test_virt.VirtTestCase)
[CPU:0.0%|MEM:17.0%]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/unit/modules/test_virt.py", line 1313, in test_update
    self.assertEqual({
AssertionError: {'definition': False, 'disk': {'attached': [], 'detached[49 chars] []}} != {'definition': True, 'disk': {'attached': [], 'detached'[74 chars]True}
+ {'cpu': True,
- {'definition': False,
? ^              ^^^^

+  'definition': True,
? ^              ^^^

   'disk': {'attached': [], 'detached': []},
-  'interface': {'attached': [], 'detached': []}}
?                                               ^

+  'interface': {'attached': [], 'detached': []},
?                                               ^

+  'mem': True}

======================================================================
```

`virt.update` falsely detects a change to the graphics setting in the
"no diff case". The old XML part specifies:

```xml
<graphics type="spice" port="5900" autoport="yes" listen="127.0.0.1">
    <listen type="address" address="127.0.0.1" />
</graphics>
```

The newly generated XML misses the port setting:

```xml
<graphics type="spice" listen="127.0.0.1" autoport="yes">
    <listen type="address" address="127.0.0.1" />
</graphics>
```

So adjust the initial XML file of the template, because setting the port
to the default 5900, will change `autoport` to `no`.

Signed-off-by: Benjamin Drung <[email protected]>
@bdrung
Copy link
Contributor Author

bdrung commented Apr 24, 2020

Fixed the lint issues.

@dwoz dwoz merged commit 0f58560 into saltstack:master Apr 24, 2020
@bdrung bdrung deleted the master-python-3.8 branch April 24, 2020 22:13
terminalmage added a commit to terminalmage/salt that referenced this pull request Apr 25, 2020
dwoz pushed a commit that referenced this pull request Apr 26, 2020
meaksh pushed a commit to openSUSE/salt that referenced this pull request Apr 30, 2020
Apply saltstack/salt#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
@waynew
Copy link
Contributor

waynew commented May 6, 2020

applies to #55835

@OrangeDog
Copy link
Contributor

Why did this bother with the try/except on collections.abc, when the old location has been deprecated since 3.3, and the minimum supported version for Sodium is 3.5?

@bdrung
Copy link
Contributor Author

bdrung commented May 19, 2020

Good point. Please open a merge request to drop falling back to the old location.

meaksh pushed a commit to openSUSE/salt that referenced this pull request May 20, 2020
Apply saltstack/salt#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
agraul pushed a commit to agraul/salt that referenced this pull request Jun 25, 2021
Apply saltstack#56031 to support Python 3.8, which removed a
deprecated module and changed some behaviour. Add a {Build,}Requires on
python-distro, since it is now required.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core relates to code central or existential to Salt ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants