-
Notifications
You must be signed in to change notification settings - Fork 120
Missing parser tests #314
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
base: master
Are you sure you want to change the base?
Missing parser tests #314
Conversation
|
There is probably some kind of regression! All tests should be passing |
|
Heh, theres some overlap here #317 |
|
Having more or less duplicated tests isn’t too bad. The real question is why it isn’t passing :/ Since it is passing as-is in pydoctor’s test cases… some troubleshooting is required |
| "mock", | ||
| "PyYAML", | ||
| "pytest", | ||
| "toml", |
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.
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.
If we want these config parses tests to run on Python versions below 3.11 we must install it on the testing environment. Please note that it stays a loose requirement to install ConfigArgParse. This is just the tests’ requirements.
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.
tomli is compatible with 3.6, pip just finds the appropriate version and installs it. Example
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.
All my tests are working with it right now over at #317
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.
Ok I understand. Did not saw the subtle change of requirement from toml to tomli
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.
Correct!
| logging.debug("%s %5d: %s" % ("**" if n2 == n else " ", n2, line)) | ||
|
|
||
| # print_source_code(test_argparse_source_code, [4540, 4565]) | ||
| for n in line_numbers: |
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.
This whole block's indentation is off!
Side note: how do you feel about using ruff in this project? And more importantly, adding it in CI as a check
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.
Strange… will look into it
I’m personally not against ruff
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’m personally not against ruff
Might others be?
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.
no objection to ruff
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.
if somebody wants to add it, please submit a PR
| # These test does not pass with TOML (even if toml support tripple quoted strings) because indentation | ||
| # is lost while parsing the config with configparser. The behaviour is basically the same as | ||
| # running textwrap.dedent() on the text. | ||
| INI_TRIPPLE_QUOTED_STRINGS_NOT_COMPATIABLE_WITH_TOML = [ |
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.
This is not used.
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.
Will check, thanks
| {'line': 'key = \n hello\n;comment\n\n hoho\n \n\n ', 'expected': ('key', ["hello", "hoho"], None)}, | ||
| ] | ||
|
|
||
| def get_IniConfigParser_cases(): |
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.
This naming is not very pythonic... a lot of the names actually.
|
|
||
| commands = python setup.py test | ||
| # python -m unittest discover | ||
| extras = |
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.
Can we move this to another PR. This has nothing to do with this PR
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.
If you want I can make a PR to get tox working with pytest, broken up by os + python version.
This adds some tests from the pydoctor test suite: https://github.com/twisted/pydoctor/blob/master/pydoctor/test/test_configparser.py