-
Notifications
You must be signed in to change notification settings - Fork 55
Add more CLI tests #152
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
Add more CLI tests #152
Conversation
Test Results 48 files ± 0 48 suites ±0 3m 25s ⏱️ +4s Results for commit 5e44eef. ± Comparison against base commit 410b987. This pull request removes 3 and adds 20 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
7bef61a to
0af9fee
Compare
|
I updated the testing to use python 3.9 python 3.6, 3.7 and 3.8 are End-Of-Life |
|
@EnricoMi ready for review now |
.github/workflows/build.yml
Outdated
| fail-fast: false | ||
| matrix: | ||
| python-version: ["3.6", "3.x"] | ||
| python-version: ["3.9", "3.x"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10+ percent of users are still on older versions so it's probably better to still have some test coverage.
f032fec to
2c275c3
Compare
|
this PR is on hold until #154 is resolved |
|
#154 once again shows why these tests are needed... |
2c275c3 to
94530a4
Compare
the command-parser for the `verify` subcommand is never used directly. But as we might want to expand the parser and for consistency, we want to keep it around
73afb1c to
029f0f6
Compare
3d59055 to
71382d1
Compare
under `lxml` the `encoding` property "UTF-8" is written in capslock, without `lxml` its lowercase. So we ignore that property and only test for the "<?xml version='1.0'" part
6ba43ab to
416d59a
Compare
5550539 to
5e44eef
Compare
|
The PR should be ready now and CI passes, not sure what to do about the codacy issue |
It complains about |
Yeah which I set to be ignored but it only seems to apply to flake8 and I am not sure why two linters are used. |
|
|
||
| # command: verify | ||
| merge_parser = command_parser.add_parser( | ||
| verify_parser = command_parser.add_parser( # noqa: F841 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume the variable is not needed if unused.
| verify_parser = command_parser.add_parser( # noqa: F841 | |
| command_parser.add_parser( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The verify_parser is still relevant and used and might be modified in the future.
Its variable binding is only unused because it currently only has the attributes inherited from the abstract_parser and no explicit ones of its own.
I believe it should be kept for consistency with the other parsers and to increase the readability for future modifications.
That's why I ignored the warning for flake8
|
Thanks for the PR. The codacy config is at the server side so it can't be skipped locally. I've fixed it for you. |
Fixes #79.