-
Notifications
You must be signed in to change notification settings - Fork 10
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
Pylint integration and more tests #17
Conversation
Codecov Report
@@ Coverage Diff @@
## main #17 +/- ##
==========================================
+ Coverage 49.53% 52.33% +2.80%
==========================================
Files 4 4
Lines 214 214
==========================================
+ Hits 106 112 +6
+ Misses 108 102 -6
Continue to review full report at Codecov.
|
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.
Thank you contributing again and great progress! I tested this PR locally and left some comments I noticed.
run: pipenv run pylint src | ||
run: | | ||
pipenv run pylint src test | ||
pipenv run pylint -d duplicate-code,invalid-name test/templates/*.py |
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.
Does disabling these do anything? I tested this locally with .pylintrc
file deleted. I only get following warnings:
Quote delimiter " is inconsistent with the rest of the file
And does above pylint run file already cover template code right?
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 still needed to disable these warnings for scripts in test/templates because otherwise my version of pylint (Ubuntu 20.04) was complaining about invalid case of variables (he wants them all uppercase) and some code duplication.
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.
Which version of pylint you are using? Maybe we should force it to specific version in Pipfile instead because of this. I mean if you get different warnings than me.
When I run pytest -v
, the first line prints following. I'm running Manjaro with i3.
platform linux -- Python 3.10.1, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /home/kazhuu/.local/share/virtualenvs/asm2cfg-954QkiKO/bin/python
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.
And also are these errors coming up in CI pipeline?
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 have
yugr@yugr-VirtualBox:~/src/my/asm2cfg$ pipenv run pylint --version
pylint 2.12.2
astroid 2.9.3
Python 3.8.10 (default, Nov 26 2021, 20:14:08)
[GCC 9.3.0]
I'm not sure how to go about these differences, maybe require a minimum Pylint version if pipenv supports this.
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.
Ah sorry the previous output doesn't even contain the version output. Here is mine:
~/programming/asm2cfg (tests/5*) » pylint --version 127 ↵ kazhuu@kazhuu
pylint 2.12.2
astroid 2.9.3
Python 3.10.1 (main, Dec 18 2021, 23:53:45) [GCC 11.1.0]
It seems we are using the same version. Wonder where you get the errors. What is the specific line you use to run pylint?
test/test_parser.py
Outdated
_, blocks = asm2cfg.parse_lines(lines, False) | ||
|
||
# TODO: special block for indirect jumps | ||
self.assertEqual(len(blocks), 2) |
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 tried to see why this test fails. I think you forgot to add split('\n')
call at the end. Then you get three blocks which seems correct to me. Could you please refactor this to be a working test.
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.
Thanks, fixed. I would still expect 4 blocks here (mind the notrack jmp
instruction).
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.
Thanks for the changes! Now the PR looks better.
No description provided.