Bug fixes, linting, drop python3.4 and Travis rework #255

Bastian-Krause · 2018-06-01T07:16:23Z

This PR includes several bug fixes. These bugs were found while using pylint. To be more consistent in coding style fix most of the linting warnings and all errors. Some warnings were left out, because the fix is either not obvious or time consuming. This is work tbd.

While at it python3.4 support was dropped.

Finally Travis-CI should check for linting errors to prevent those bugs from now on.

I think more warnings and errors should be ignored, but we need a separate discussion for that. Some warnings are caused by pylint not recognizing attr behavior. Discussion happens in #257.

codecov-io · 2018-06-01T09:36:57Z

Codecov Report

Merging #255 into master will decrease coverage by 0.3%.
The diff coverage is 47.4%.

@@           Coverage Diff            @@
##           master    #255     +/-   ##
========================================
- Coverage    54.9%   54.5%   -0.4%     
========================================
  Files         105     105             
  Lines        6229    6181     -48     
========================================
- Hits         3424    3374     -50     
- Misses       2805    2807      +2

Impacted Files	Coverage Δ
labgrid/util/agentwrapper.py	`100% <ø> (ø)`	⬆️
labgrid/resource/ykushpowerport.py	`100% <ø> (ø)`	⬆️
labgrid/protocol/commandprotocol.py	`100% <ø> (ø)`	⬆️
labgrid/resource/serialport.py	`94.1% <ø> (ø)`	⬆️
labgrid/driver/smallubootdriver.py	`42.4% <ø> (-8.9%)`	⬇️
labgrid/util/expect.py	`76.4% <ø> (-2.5%)`	⬇️
labgrid/protocol/linuxbootprotocol.py	`100% <ø> (ø)`	⬆️
labgrid/pytestplugin/reporter.py	`34.5% <0%> (ø)`	⬆️
labgrid/driver/powerdriver.py	`69.3% <0%> (-0.2%)`	⬇️
labgrid/remote/authenticator.py	`0% <0%> (ø)`	⬆️
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8289085...ed61360. Read the comment docs.

Emantor · 2018-06-04T08:39:48Z

Do you have any idea why coverages takes a nosedive?

Bastian-Krause · 2018-06-04T08:49:58Z

Do you have any idea why coverages takes a nosedive?

Yes, this is mostly caused by "drop python3.4 support" and "linting: do miscellaneous linting". These commits remove lines, either by dropping the @asyncio.coroutine decorator line on tested code or by
reformatting (joining lines of tested code or adding line breaks in untested code). See code coverage drops in:

Bastian-Krause · 2018-06-04T15:03:17Z

rebased
drop "factory: normalize config with correct driver class" in favor of merged minor factory fixes #258
sort "strategy/graphstrategy: simplify dict iteration in graph()" after "linting: strategy/graphstrategy: no empty list as arg default" and prefix with "linting: "

Bastian-Krause · 2018-06-04T15:50:30Z

rebased
ignore linting errors when emitting exit code

Bastian-Krause · 2018-06-04T16:06:22Z

tox.ini: use leading minus instead of ignore_errors=True in order not to influence the tox exit code

jluebbe

Please leave out the tox-related changes for now. I'd like to do my own experiments with it first.

jluebbe · 2018-06-04T16:15:34Z

labgrid/autoinstall/main.py

@@ -73,7 +73,7 @@ def run(self):
            self.context['target'] = self.target
            if self.target is None:
                raise KeyError
-        except:
+        except KeyError:


Other exceptions can be generated by env.get_target() during target creation, which should also be caught by this catch all.

jluebbe · 2018-06-04T16:20:18Z

labgrid/target.py

        """
        Helper function to get a resource of the target.
        Returns the first valid resource found, otherwise None.

        Arguments:
        cls -- resource-class to return as a resource
        name -- optional name to use as a filter
-        await -- wait for the resource to become available (default True)
+        wait_for -- wait for the resource to become available (default True)


I'd prefer wait_avail instead of wait_for. wait_for suggests that you need to pass the thing you want to wait for.

jluebbe · 2018-06-04T16:22:39Z

labgrid/remote/exporter.py

@@ -381,13 +381,9 @@ def onChallenge(self, challenge):
        self.loop.stop()

    async def acquire(self, group_name, resource_name):
-        resource = self.groups[group_name][resource_name]


This (and the one below) should actually be TODOs. We want to be able to perform local actions when a resource is acquired, and this would be the correct place when we get around to that.

jluebbe · 2018-06-04T16:29:18Z

labgrid/driver/shelldriver.py

@@ -61,13 +61,14 @@ def on_activate(self):
            self._inject_run()
        if self.keyfile:
            self._put_ssh_key(self.keyfile)
-        self._run("dmesg -n 1")  # Turn off Kernel Messages to the console
+        # Turn off Kernel Messages to the console
+        self._run("dmesg -n 1")  # pylint: disable=missing-kwoa


Please disable missing-kwoa and unused-argument for the whole file. The annotations are too noisy.

jluebbe · 2018-06-04T16:30:31Z

labgrid/remote/common.py

@@ -103,8 +103,8 @@ class Place:
    acquired = attr.ib(default=None)
    acquired_resources = attr.ib(default=attr.Factory(list))
    allowed = attr.ib(default=attr.Factory(set), convert=set)
-    created = attr.ib(default=attr.Factory(lambda: time.time()))
-    changed = attr.ib(default=attr.Factory(lambda: time.time()))
+    created = attr.ib(default=attr.Factory(lambda: time.time()))  # pylint: disable=unnecessary-lambda


This message seems correct, we should just use Factory(time.time).

jluebbe · 2018-06-04T16:30:56Z

labgrid/remote/common.py

@@ -16,27 +16,27 @@ def __attrs_post_init__(self):

    @property
    def avail(self):
-        return self.data['avail']
+        return self.data['avail']  # pylint: disable=unsubscriptable-object


Disable this for the whole file.

…erFoundError Signed-off-by: Bastian Stender <bst@pengutronix.de>