bpo-29812: Improve testing of token and tokenize by ammaraskar · Pull Request #681 · python/cpython

ammaraskar · 2017-03-16T02:19:27Z

Adds a token.py generation test, and a cross-check between the token and tokenize modules.

The generation test is akin to the current automated file generator tests in test_keyword and test_symbol. The newly added check in tokenize ensures that all the literal tokens in token such as ':', '...', '@=' have a corresponding entry within the EXACT_TOKEN_TYPES dict in tokenize.

mention-bot · 2017-03-16T02:19:30Z

@ammaraskar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @tiran and @1st1 to be potential reviewers.

ammaraskar · 2017-03-16T02:59:58Z

Weirdly enough this fails on AppVeyor (which is Windows, I believe). I'm not sure if this is something wrong with the test or an actual problem on windows, I'll look into it tomorrow when I have access to a windows computer.

berkerpeksag · 2017-04-11T11:17:18Z

@@ -0,0 +1,52 @@
+import token
+import unittest
+from test import support


Please group stdlib modules together and sort them alphabetically.

berkerpeksag · 2017-04-11T11:18:44Z

+import sys
+import subprocess
+
+TOKEN_FILE               = support.findfile('token.py')


Style nit: Don't add additional spaces before =.

berkerpeksag · 2017-04-11T11:21:02Z

+        rc, stderr = self._generate_tokens(os.devnull, NONEXISTENT_FILE)
+        self.assertNotEqual(rc, 0)
+        self.assertIn(b'I/O error', stderr)
+        self.assertIn(bytes(NONEXISTENT_FILE, encoding='ascii'), stderr)


NONEXISTENT_FILE doesn't contain any non-ASCII chars. Do we really need encoding='ascii'?

Also, I prefer to have a nonexistent_file local variable in the test instead of a constant.

The encoding is required when passing a string in python3: https://docs.python.org/3/library/functions.html#bytes
https://docs.python.org/3/library/functions.html#bytearray

The reason its a b'string' is because it needs to be passed as an argument to Popen, changing it to a local might result in an easier fix for this though.

berkerpeksag · 2017-04-11T11:23:26Z

+        self.assertIn(bytes(NONEXISTENT_FILE, encoding='ascii'), stderr)
+
+
+if __name__ == "__main__":


Style use: Please use single quotes where possible.

Will do, for reference, this was copied from https://github.com/python/cpython/blob/master/Lib/test/test_keyword.py

berkerpeksag · 2017-04-11T11:24:12Z

+        stderr = proc.communicate()[1]
+        return proc.returncode, stderr
+
+    @unittest.skipIf(not os.path.exists(TOKEN_FILE),


I'd replace

@unittest.skipIf(not condition)

with

@unittest.skipUnless(condition)

berkerpeksag · 2017-04-11T11:25:55Z

+                     'test only works from source build directory')
+    def test_real_token_file(self):
+        self._copy_file_without_generated_tokens(TOKEN_FILE, TEST_PY_FILE)
+        self.addCleanup(support.unlink, TEST_PY_FILE)


You can probably move the addCleanup call to inside of _copy_file_without_generated_tokens method.

berkerpeksag · 2017-04-11T11:26:53Z

+                                                         TEST_PY_FILE))
+        self.assertTrue(filecmp.cmp(TOKEN_FILE, TEST_PY_FILE))
+
+    def test_missing_output_file_causes_Error(self):


Error -> error or exception

berkerpeksag · 2017-04-11T11:28:13Z

                         token.tok_name[token.ENDMARKER])

+    def test_all_literal_tokens(self):
+        NON_LITERALS = {token.ENDMARKER, token.NAME, token.NUMBER,


NON_LITERALS -> non_literals or non_literal_tokens

berkerpeksag · 2017-04-11T11:30:59Z

                         token.tok_name[token.ENDMARKER])

+    def test_all_literal_tokens(self):
+        NON_LITERALS = {token.ENDMARKER, token.NAME, token.NUMBER,


Also, I find hanging indents much more readable and they will make future diffs less noisy:

non_literal_tokens = { token.STRING, token.NEWLINE, token.INDENT, ..., # You can add another line without having to modify the '}' so diffs will be less noisy. token.N_TOKENS, token.NT_OFFSET, }

berkerpeksag · 2017-04-11T11:33:10Z

+                continue
+            name = tok_name[tok]
+            self.assertIn(tok, EXACT_TOKEN_TYPES.values(),
+                    "Literal token " + name + " not in EXACT_TOKEN_TYPES")


This is OK, but an alternative solution would be to use self.subTest().

ammaraskar · 2017-04-11T20:43:41Z

Oh also, I tried to look into what was calling the AppVeyor failure when comparing the files. From what I can tell its because os.stat is returning an incorrect value for st_size on the test environment or something https://ci.appveyor.com/project/ammaraskar/cpython/build/1.0.9#L3949

Doing a file content comparison causes it to pass.

Nevermind, I don't think this is an incorrect return since the offset of the size is exactly the number of lines, I think this has to do with line endings.

Adds a token.py generation test, and a cross-check between the token and tokenize modules. The generation test is akin to the current automated file generator tests in test_keyword and test_symbol. The newly added check in tokenize ensures that all the literal tokens in token such as ':', '...', '@=' have a corresponding entry within the EXACT_TOKEN_TYPES dict in tokenize.

Currently this code does not work on windows when the skeleton file has unix line endings because fp.write('\n') will have the LF changed to CRLF due to python's newline interpolation.

ammaraskar · 2017-06-25T20:06:01Z

Hey @berkerpeksag, could you take a look at this, I updated it fixing the concerns in your review.

@bitdancer could I also get a review from you seeing as you made the issue on the tracker for this?

berkerpeksag

Sorry for my late response. I left some comments. Please add a NEWS entry in Misc/NEWS.d/next/Tests.

Out of curiosity, how did you notice 3d4a043? It looks like AppVeyor didn't catch it.

berkerpeksag · 2017-07-03T03:25:37Z

+import os
+import subprocess
+import sys
+from test import support


Please split stdlib and non-stdlib modules.

berkerpeksag · 2017-07-03T03:30:11Z

+        self.addCleanup(support.unlink, dest_file)
+
+    def _generate_tokens(self, skeleton_file, target_token_py_file):
+        proc = subprocess.Popen([sys.executable,


Can't we use the new subprocess.run() interface?

berkerpeksag · 2017-07-03T03:34:29Z

    with fp:
-        format = fp.read().split("\n")
+        format = fp.readlines()
+    nl = format[0][len(format[0].strip()):] if format else '\n'


I'd use more descriptive name like newline.

berkerpeksag · 2017-07-03T03:44:24Z

+                continue
+            name = tok_name[tok]
+            with self.subTest(name=name):
+                self.assertIn(tok, EXACT_TOKEN_TYPES.values())


Looking at CI logs of an earlier commit, I wonder if we should make the following message clearer:

====================================================================== FAIL: test_all_literal_tokens (test.test_tokenize.TestTokenize) (name='NL') ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/travis/build/python/cpython/Lib/test/test_tokenize.py", line 1368, in test_all_literal_tokens self.assertIn(tok, EXACT_TOKEN_TYPES.values()) AssertionError: 58 not found in dict_values([7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 52, 51, 49, 50])

ammaraskar · 2017-07-12T02:23:28Z

Thanks for your review @berkerpeksag. Closing this as per discussion on bpo.

the-knights-who-say-ni added the CLA signed label Mar 16, 2017

ammaraskar force-pushed the master branch 2 times, most recently from 5c9e66a to dbce4b5 Compare March 16, 2017 02:49

berkerpeksag requested changes Apr 11, 2017

View reviewed changes

ammaraskar force-pushed the master branch 3 times, most recently from 963035d to 9b357ae Compare April 11, 2017 20:26

ammaraskar added 2 commits June 24, 2017 14:39

Changes for berker's review

635f0e7

ammaraskar force-pushed the master branch from 7e57383 to f4ebfe1 Compare June 24, 2017 21:41

ammaraskar mentioned this pull request Jun 24, 2017

bpo-25324: copy tok_name before changing it #1608

Merged

ammaraskar force-pushed the master branch from f4ebfe1 to bc9a3c1 Compare June 24, 2017 22:12

Preserve newline conventions for skeleton file

3d4a043

Currently this code does not work on windows when the skeleton file has unix line endings because fp.write('\n') will have the LF changed to CRLF due to python's newline interpolation.

ammaraskar force-pushed the master branch from bc9a3c1 to 3d4a043 Compare June 24, 2017 22:23

Add newly added non literal tokens to test

7521597

berkerpeksag reviewed Jul 3, 2017

View reviewed changes

ammaraskar closed this Jul 12, 2017

		self.assertIn(bytes(NONEXISTENT_FILE, encoding='ascii'), stderr)


		if __name__ == "__main__":

Uh oh!

Uh oh!

Conversation

ammaraskar commented Mar 16, 2017

Uh oh!

mention-bot commented Mar 16, 2017

Uh oh!

ammaraskar commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ammaraskar Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ammaraskar commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ammaraskar commented Jun 25, 2017

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ammaraskar commented Jul 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ammaraskar Apr 11, 2017 •

edited

Loading

ammaraskar commented Apr 11, 2017 •

edited

Loading