Opened on 01/07/2019 at 12:45:39 PM
Closed on 08/29/2019 at 04:20:07 PM
#7194 closed defect (worksforme)
tox is failing for buildtools, reporting flake8 errors
Reported by: | kzar | Assignee: | |
---|---|---|---|
Priority: | Unknown | Milestone: | |
Module: | Automation | Keywords: | |
Cc: | tlucas, sebastian | Blocked By: | #7196 |
Blocking: | Platform: | Unknown / Cross platform | |
Ready: | no | Confidential: | no |
Tester: | Unknown | Verified working: | no |
Review URL(s): |
Description (last modified by kzar)
Environment
- buildtools 33f115e8c1b3, tox 3.6.1, Debian testing.
How to reproduce
- Clone the buildtools repository, run tox.
Observed behaviour
While the tests all seem to pass, the following is output at the end:
py27 runtests: commands[2] | flake8 ./releaseAutomation.py:146:17: W504 line break after binary operator ./releaseAutomation.py:151:17: W504 line break after binary operator ./ensure_dependencies.py:370:13: W504 line break after binary operator ./jshydra/abp_rewrite.py:29:33: C812 missing trailing comma ./jshydra/abp_rewrite.py:31:20: C812 missing trailing comma ./jshydra/abp_rewrite.py:47:39: C812 missing trailing comma ./jshydra/abp_rewrite.py:73:5: E722 do not use bare 'except' ./jshydra/autotest.py:58:1: E305 expected 2 blank lines after class or function definition, found 1 ERROR: InvocationError for command '/path/to/adblockpluschrome/buildtools/.tox/py27/bin/flake8' (exited with code 1)
Expected behaviour
Those error messages are not displayed.
Notes
- Looking at it, jshydra is only there as it used to be a dependency of buildtools, and is also therefore being ignored by Git. Nevertheless, I figure we should still tell flake8 to skip checking it.
- It seems that while newer versions of flake8 enforce the W504 line break after binary operator, older versions actually enforce the opposite W503 line break before binary operator rule! We've configured tox to use both a recent and old version of flake8, so we'll have to ignore at least one of these rules.
Attachments (0)
Change History (7)
comment:3 Changed on 01/07/2019 at 02:03:44 PM by kzar
- Blocked By 7196 added
comment:4 Changed on 01/07/2019 at 02:18:42 PM by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:5 in reply to: ↑ description Changed on 01/07/2019 at 11:25:18 PM by sebastian
comment:6 Changed on 01/08/2019 at 10:15:53 AM by kzar
Sebastian requested we move the discussion about ignoring the jshydra directory here. First of all, I'll repeat my initial point from the codereview:
"Why not just delete the jshydra directory?"
Well, it'll keep getting created again when I am bisecting with old versions of the code.
Then to respond to Sebastian's argument:
The jsHydra dependency has been removed ages ago, and it feels like a hack to account for it in tox.ini going forward.
Well, I kind of disagree. For me this directory will keep showing up, and we never want to lint it, so explicitly ignoring it from linting makes sense. Also the change we're talking about is trivial, and unlikely to cause confusion:
-
tox.ini
diff --git a/tox.ini b/tox.ini index 2b2c33d..1673374 100644
a b skipsdist = true 6 6 7 7 exclude = 8 8 node_modules, 9 jshydra, 9 10 .tox, 10 11 *.pyc, 11 12 ignore = D1,C815
comment:7 Changed on 08/29/2019 at 04:20:07 PM by sebastian
- Resolution set to worksforme
- Status changed from reviewing to closed
The W504 errors have been addressed by now. The issue when switching from an old revision that still used jsHydra to a modern revision of buildtools remains, but with buildtools on the way out, I'm closing this issue now.
Replying to kzar:
The jsHydra dependency has been removed ages ago, and it feels like a hack to account for it in tox.ini going forward. But I admit ensure_dependencies.py keep ignoring gone dependencies isn't ideal. Ideally, hg status/git status should indicate and hg clean/git clean -f -d should remove untracked files from obsolete dependencies.
For reference, this is being discussed here.