Fix part of #12912: Enabling linter consider-using-with #20134
Conversation
|
Assigning @lkbhitesh07 for the first pass review of this PR. Thanks! |
|
Hey @m0nax5 thanks for the PR, could you please fix the failing tests and then assign me again? |
|
Hi ! Yes I'm currently working on it |
|
Hi @lkbhitesh07, I fixed the linter issues that were about I looked into it and from what I understand there is error in finding filepaths, I believe due to an issue in decoding utf8. A similar error also occurred in the backend tests that automatically ran after the last time I pushed my code : each time there is a wrong folder name in the middle of the filepath such as in I don't really know how to handle this issue, except from the fact that the problem comes from utf8 decoding and that it probably stems from my changes to the file scripts/extend_index_yaml_test.py. Could you please give me some guidance regarding the issue ? |
|
Assigning @U8NWXD, @lkbhitesh07 for code owner reviews. Thanks! |
|
Since this PR is going to change when you fix the CI checks, please re-assign me for review once those are passing |
|
Hi @U8NWXD, I have tried several different syntaxes to solve my UTF-8 encoding issue in diff --git a/scripts/extend_index_yaml_test.py b/scripts/extend_index_yaml_test.py
index 4264837a3d..2523cfd479 100644
--- a/scripts/extend_index_yaml_test.py
+++ b/scripts/extend_index_yaml_test.py
@@ -19,6 +19,7 @@
from __future__ import annotations
import tempfile
+import unittest
from core.tests import test_utils
@@ -168,38 +169,51 @@ class ReformatXmlToYamlTests(test_utils.GenericTestBase):
)
-class ExtendIndexYamlTests(test_utils.GenericTestBase):
+class ExtendIndexYamlTests(unittest.TestCase):
"""Class for testing the extend_index_yaml script."""
def _run_test_for_extend_index_yaml(
self, index_yaml: str, web_inf_index_xml: str, expected_index_yaml: str
) -> None:
"""Run tests for extend_index_yaml script."""
- with tempfile.NamedTemporaryFile() as index_yaml_file:
- with tempfile.NamedTemporaryFile() as web_inf_index_xml_file:
- with self.swap(
- extend_index_yaml, 'INDEX_YAML_PATH',
- index_yaml_file.name
- ):
- with self.swap(
- extend_index_yaml, 'WEB_INF_INDEX_XML_PATH',
- web_inf_index_xml_file.name
- ):
- with open(
- index_yaml_file.name, 'w', encoding='utf-8'
- ) as open_index_yaml_w:
- open_index_yaml_w.write(index_yaml)
- with open(
- web_inf_index_xml_file.name, 'a', encoding='utf-8'
- ) as open_web_inf_index_xml:
- open_web_inf_index_xml.write(web_inf_index_xml)
- extend_index_yaml.main()
- with open(
- index_yaml_file.name, 'r', encoding='utf-8'
- ) as open_index_yaml_r:
- actual_index_yaml = open_index_yaml_r.read()
- self.assertEqual(
- actual_index_yaml, expected_index_yaml)
+ original_yaml_pth = extend_index_yaml.INDEX_YAML_PATH
+ original_xml_pth = extend_index_yaml.WEB_INF_INDEX_XML_PATH
+
+ with tempfile.NamedTemporaryFile(
+ mode='w+', encoding='utf-8'
+ ) as tmp_index_yaml:
+ extend_index_yaml.INDEX_YAML_PATH = tmp_index_yaml.name
+ with tempfile.NamedTemporaryFile(
+ mode='w+', encoding='utf-8'
+ ) as tmp_index_xml:
+ extend_index_yaml.WEB_INF_INDEX_XML_PATH = tmp_index_xml.name
+
+ try:
+ with open(
+ extend_index_yaml.INDEX_YAML_PATH, 'w',
+ encoding='utf-8'
+ ) as f:
+ f.write(index_yaml)
+
+ with open(
+ extend_index_yaml.WEB_INF_INDEX_XML_PATH, 'a',
+ encoding='utf-8'
+ ) as f:
+ f.write(web_inf_index_xml)
+
+ extend_index_yaml.main()
+
+ with open(
+ extend_index_yaml.INDEX_YAML_PATH, 'r',
+ encoding='utf-8'
+ ) as f:
+ actual_index_yaml = f.read()
+
+ self.assertEqual(actual_index_yaml, expected_index_yaml)
+
+ finally:
+ extend_index_yaml.INDEX_YAML_PATH = original_yaml_pth
+ extend_index_yaml.WEB_INF_INDEX_XML_PATH = original_xml_pth
def test_extend_index_yaml_with_changes(self) -> None:
index_yaml = """indexes:The same kind of failures as in my previous comment appeared in each of my attempts to run the linter checks, ie. unable to find filepaths due to unreadable UTF-8 characters. Yet appart from that the lint tests pass. I would really appreciate if you could provide me with some guidance to tackle this, especially if you have any idea why some UTF-8 characters are not decoded well (since I didn’t change anything in the bits related to UTF-8 encoding). Thanks for your help ! |
|
From I recommend investigating why the linter is trying to read this file even though we should be excluding it from the lint checks since it's not a code file |
|
Thanks for the tip ! You were right I was able to pass all linter checks once I had manually removed all the |
There was a problem hiding this comment.
Hey @m0nax5, LGTM for the code owner files. Thanks for the changes, I have left a comment regarding the failed backend tests. I hope it will help!
Thanks!
| frontend_file = os.path.join(tempdir.name, 'frontend_file.ts') | ||
| with tempfile.TemporaryDirectory( | ||
| prefix=os.getcwd() + '/core/') as tempdir: | ||
| backend_file = os.path.join(tempdir, 'backend_file.py') |
There was a problem hiding this comment.
Hey @m0nax5, for the backend test, I think here it should be tempdir.name, could you please check if after this the backend tests passes, same changes goes for the other backend test below as well.
Thanks
There was a problem hiding this comment.
Hi @lkbhitesh07, I implemented the changes that you suggested accross the file and the backend tests still won't pass. Here is the log I got for the backend tests :
======================================================================
ERROR: test_checks_fail_when_a_backend_file_lacks_associated_test_file (scripts.check_backend_associated_test_file_test.CheckBackendAssociatedTestFileTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/opensource2/oppia/scripts/check_backend_associated_test_file_test.py", line 54, in test_checks_fail_when_a_backend_file_lacks_associated_test_file
with open(backend_file, 'w', encoding='utf8') as f:
FileNotFoundError: [Errno 2] No such file or directory: '/home/opensource2/oppia/core/z0twcbrp/backend_file.py'
======================================================================
ERROR: test_checks_pass_when_all_backend_files_have_an_associated_test_file (scripts.check_backend_associated_test_file_test.CheckBackendAssociatedTestFileTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/opensource2/oppia/scripts/check_backend_associated_test_file_test.py", line 97, in test_checks_pass_when_all_backend_files_have_an_associated_test_file
backend_file = os.path.join(tempdir.name, 'backend_file.py')
AttributeError: 'str' object has no attribute 'name'
======================================================================
ERROR: test_pass_when_file_in_exclusion_list_lacks_associated_test (scripts.check_backend_associated_test_file_test.CheckBackendAssociatedTestFileTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/opensource2/oppia/scripts/check_backend_associated_test_file_test.py", line 76, in test_pass_when_file_in_exclusion_list_lacks_associated_test
backend_file = os.path.join(tempdir.name, 'backend_file.py')
AttributeError: 'str' object has no attribute 'name'
---------------------------------------------------------------------
The first error is an occurrence of the previous syntax without .name that I left on purpose, whereas the two last errors are what I get when I add .name. What I don't understand is why when using the with clause tempdir has type str, whereas when it is declared with tempdir = tempfile.TemporaryDirectory() it has type tempfile.TemporaryDirectory and thus tempdir.name makes sense and has type str. I'm currently looking into finding a solution to this issue.
Thanks again for the review !
|
Assigning @U8NWXD for code owner reviews. Thanks! |
|
De-assigning myself for now since this PR still needs CI fixes |
|
Hi @m0nax5, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
|
Hi @m0nax5, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
|
Hi @m0nax5, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Overview
consider-using-withagain by fixing every instance of linter check failure in the existing codebase, by introducing with statements when they are needed.Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
After removing 'consider-using-with' from the disabled linters, the linter output was :
Now after the changes from this PR all linter checks pass :
PR Pointers