Assert messages generated by a python lint check
gp201 opened this issue · 24 comments
As per the discussion in #12394 (comment) the message produced by pylint in the terminal must be asserted.
Ex:
This message Please use python_utils.with_metaclass() instead of __metaclass__ is generated when the presence of __metaclass__ is found in a file.
This issue's aim is to add an assertion to the test in pylint_extensions_test.py to check if the message is generated. Currently, only the message ID is being asserted.
Additional context
Add any other context about the problem here.
Hi @oppia/core-maintainers, this issue is not assigned to any project. Can you please update the same? Thanks!
Please assign me this issue!
@agarwaldevesh374 Per the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue, please provide an explanation of what your PR will do (with names of files you're changing, what you plan to change in each file, etc.). If it looks good, we can assign you to this issue.
Please also follow the other instructions on that wiki page if you have not yet done so. Thanks!
Greetings, I would like to know if this is still open.
@vivekdhir77 I imagine it is, unless you find evidence to the contrary.
If you would like to tackle it, please see my reply to @agarwaldevesh374 above.
Hey can i be assigned this issue?
I'll be fixing issue #12507 by adding an assertion to the test in pylint_extensions_test.py to check if the message is generated.
I've also followed the rules on the wiki page :)
I am unable to find the class DisallowDunderMetaclassCheckerTests , has it been removed ? Tried searching the content of
with checker_test_object.assertAddsMessages(
testutils.Message(
msg_id='no-dunder-metaclass',
couldn't find any of it.
What am i dong wrong?Because i can see it's PR being merged as per the discussion in #12394
@SahilPrabhu Thanks for your message. Which test are you modifying exactly and what are you planning to change?
@satyamsaxena2001 Good question! Looking at the history of the file, it seems to have been removed in #13998.
However, the original idea of asserting messages and not just IDs might still apply to the other lint checks in that file. If so, then we can keep this issue open and open a PR for that.
@seanlip
Is this what is expected in this issue for all similar id checks?
Commented part is the initial code,below that i have tried adding assertion check
@seanlip opinions?
@satyamsaxena2001 I'm not really sure what you're doing, to be honest. I think the point of the issue is to verify that the error message surfaced by the custom linter matches the expected error message. I.e. you would run the lint function against some code, get the error message that that produces, and compare it against an expected string that represents what you would expect the message to be. (See the description in the original issue.)
I can't see any of the above conceptually in your screenshot, so I can't tell what is going on.
@Gauravkumar2701 Please see #12507 (comment) for instructions on how to claim an issue.
@seanlip Well,I was wondering if can we use lint.run method to output message in a text file and then use regex to extract message and then compare and assert the message. I tried doing it for a dummy hello world token checker and it seemed to work. Am I going in the right direction.
@sunilkumardash9 It would be helpful if you could show an example for an existing lint check in the Oppia codebase. It's hard to review something completely outside the repo because the context is missing.
@seanlip Yeah so, I tried to add a message check function to a standalone hanging indent checker (a part of pylint_extensions.py) as you can see in this file.
the message check function
def assert_msg(self, filename):
msg_str = 'There should be a break after parenthesis when content within parenthesis spans multiple lines.'
msg_id = 'no-break-after-hanging-indent'
with open('pylint_output.txt', 'w') as f:
args = ['--load-plugins', 'hanging_indent_check',"--disable",'all','--enable',msg_id,filename]
lint.Run(args, reporter=TextReporter(f), exit=False)
with open('pylint_output.txt', 'r') as f:
output = f.read()
print(output,)
id = 'C0002'
regex = id+r": (.*)\."
match = re.search(regex, output)
self.assertEqual(msg_str, match.group(1)+'.')
But there seems to be a problem. And the problem is whenever I test the checker test file with unittest with the following file,
with utils.open_file(filename,'w') as tmp:
tmp.write(
u"""self.post_json ('/ml/\\trainedclassifierhandler',
self.payload, expect_errors=True, expected_status_int=401)
if (a>1 and
b > 2):
""")
I do not get the intended message from pylint i.e. C0002: There should be a break after parenthesis when content within parenthesis spans multiple lines. (no-break-after-hanging-indent) .
But if I exclude the if block then the entire thing works. see below
But if I try to recreate the test separately with the checker and a test file
self.post_json('/ml/\\trainedclassifierhandler',
self.payload, expect_errors=True, expected_status_int=401)
if (a>b and
b>a):pass
It shows the message in the terminal.
Not really sure, it it's an intended behaviour or not.
def test_pylint_message():
pylint_output = run_pylint_on_file('example_file.py')
expected_message = "E0012: Please use python_utils.with_metaclass() instead of metaclass"
assert expected_message in pylint_output
This test will run pylint on the example file and capture the output. It will then check that the expected message is present in the output by searching for both the message ID and the message content.
@sunilkumardash9 I would suggest that you make changes to pylint_extensions_test.py directly and test that, rather than calling unittest directly. This will help build confidence in your solution. Note that the wiki has instructions for how to run tests for specific files locally.
It's a bit hard to comment on the rest of your solution because it is missing certain details. E.g. you say "I do not get the intended message from pylint" but you don't explain what you got. I feel like reading our documentation on backend tests and working within the existing infrastructure would help bridge the gap, so please do check out the wiki.
@GOVINDFROMINDIA While this approach may work, it would be great to see if you can integrate it with the existing codebase. If you can provide a screenshot showing a local run with the expected behaviour for one of the checks, we can assign you the issue.
I could make it work for first checker.
Do we need to assert in cases where it is not supposed to produce any message?
It can be done for token checkers by just passing the file to the function that will assert the message. For the Astroid checkers a separate file needs to be created with 'open'.
@DubeySandeep what was the rationale behind this change? I don't see how a lint check could raise an issue with the correct ID but incorrect message
@U8NWXD I cannot recall the actual requirement but I think it will help us ensure the exact text/pattern of the reported message and to ensure one can easily follow the reported message.
Assign this to me . . .
Closing since it doesn't appear to be needed