Issue 4895499043733504: Issue 510 - [Typed objects] Don`t hardcode script load order in unit tests (Closed)
|
Created:
Jan. 10, 2015, 12:13 a.m. by Wladimir Palant Modified:
Feb. 5, 2015, 3:16 p.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 510 - [Typed objects] Don`t hardcode script load order in unit tests
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed comments #
Total comments: 5
Patch Set 3 : Reverted to original get_scripts implementation #Patch Set 4 : Added comment #
Total comments: 2
Patch Set 5 : Using generators to produce responses #
MessagesTotal messages: 16
http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/.hgs... File .hgsubstate (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/.hgs... .hgsubstate:1: d357001ed1e487cd8854d91370d71bd66c885ef6 jshydra This dependency update is unrelated, I extracted it into a separate commit now.
Looks pretty good. Considering that we're still hard coding script load order in libadblockplus (and I presume some other places as well), it would be nice to have this as part of adblockpluscore somehow. But that's something for a follow-up. http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... run_tests.py:22: import io Nit: The import statements seem to be ordered alphabetically. http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... run_tests.py:27: def get_scripts(dir, reldir = ""): This would be a bit shorter (and IMHO easier to read) using os.walk and fnmatch, e.g.: def get_scripts(dir): for root, _, files in os.walk(dir): for file in fnmatch.filter(files, "*.js"): path = os.path.join(root, file) relpath = "/" + os.path.relpath(path, dir) yield(path, relpath) http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... run_tests.py:39: .replace("\r", "\\r").replace("\n", "\\n")) Makes me wonder if we'll run into trouble when using other control sequences than \r or \n. Assuming JS supports the same control sequences as C, this should handle all of them: re.sub(r"\([a-z0-9])", r"\\\1", ...) http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... File test/common.js (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... test/common.js:39: require.modules = {}; Have you considered calling this "sources"? Seems clearer to me that way.
http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... File test/index.html (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... test/index.html:25: evalScript = function(source) Missed this before: Why are evalScript and evalModule redefined to what seems like identical functions here?
http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... File test/index.html (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... test/index.html:25: evalScript = function(source) On 2015/02/03 06:30:24, Felix H. Dahlke wrote: > Missed this before: > > Why are evalScript and evalModule redefined to what seems like identical > functions here? Oh nevermind that, to run it as JS 1.7 code of course...
http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... run_tests.py:22: import io On 2015/02/03 06:21:16, Felix H. Dahlke wrote: > Nit: The import statements seem to be ordered alphabetically. Note that according to PEP-8, third-party module imports (like flask here) should go below core-lib imports. http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... run_tests.py:27: def get_scripts(dir, reldir = ""): On 2015/02/03 06:21:16, Felix H. Dahlke wrote: > This would be a bit shorter (and IMHO easier to read) using os.walk and fnmatch, > e.g.: > > def get_scripts(dir): > for root, _, files in os.walk(dir): > for file in fnmatch.filter(files, "*.js"): > path = os.path.join(root, file) > relpath = "/" + os.path.relpath(path, dir) > yield(path, relpath) Even more compact: import glob ... def get_scripts(dir): for filename in glob.glob(os.path.join(dir, '*.js')): ... http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... File test/index.html (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... test/index.html:27: return eval(source); Do you ever plan to run those tests as part of a Chrome extension? Note that Chrome extensions cannot use eval().
http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... run_tests.py:22: import io On 2015/02/03 09:21:10, Sebastian Noack wrote: > On 2015/02/03 06:21:16, Felix H. Dahlke wrote: > > Nit: The import statements seem to be ordered alphabetically. > > Note that according to PEP-8, third-party module imports (like flask here) > should go below core-lib imports. Done. http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... run_tests.py:27: def get_scripts(dir, reldir = ""): On 2015/02/03 09:21:10, Sebastian Noack wrote: > for filename in glob.glob(os.path.join(dir, '*.js')): This won't find anything in subdirectories. Personally, I don't find os.walk() easier to read - the calling convention of that function is everything but natural. Note also that os.path.relpath() will produce OS-specific paths which isn't what we need here. However, I've implemented it this way now. http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/run_... run_tests.py:39: .replace("\r", "\\r").replace("\n", "\\n")) On 2015/02/03 06:21:16, Felix H. Dahlke wrote: > Makes me wonder if we'll run into trouble when using other control sequences > than \r or \n. No, these should be fine within a string. See http://www.ecma-international.org/ecma-262/5.1/#sec-7.8.4: > SourceCharacter but not one of ' or \ or LineTerminator So these are the only characters that need special treatment. However, LineTerminator also includes \u2028 and \u2029. Either way, using json module would be a cleaner solution. http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... File test/common.js (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... test/common.js:39: require.modules = {}; On 2015/02/03 06:21:16, Felix H. Dahlke wrote: > Have you considered calling this "sources"? Seems clearer to me that way. Done. http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... File test/index.html (right): http://codereview.adblockplus.org/4895499043733504/diff/5629499534213120/test... test/index.html:27: return eval(source); On 2015/02/03 09:21:10, Sebastian Noack wrote: > Do you ever plan to run those tests as part of a Chrome extension? The tests themselves - yes, sure. However, the framework running them would be different. I guess that for Chrome we will put the code into anonymous functions rather than strings, then call these functions when necessary. This will make debugging (mapping to original files) a lot more complicated unfortunately.
http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... run_tests.py:33: path = os.path.join(dir, file) posixpath.join?
http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... run_tests.py:25: import flask While we're nit picking :) PEP-8: "You should put a blank line between each group of imports."
http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... run_tests.py:33: path = os.path.join(dir, file) On 2015/02/04 03:58:51, Felix H. Dahlke wrote: > posixpath.join? No, we need a proper OS-specific path here, only the relative path needs to use forward slashes. But now that you say it - relative path calculation below won't work correctly on Windows, not even with posixpath. So far I've come up with the following monster: def split_path(path): path, filename = os.path.split(path) return (split_path(path) if path else []) + [filename] ... relpath = posixpath.join(*split_path(os.path.relpath(path, root))) I think I better switch back to the original recursive approach.
LGTM, one naming suggestion. http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... run_tests.py:33: path = os.path.join(dir, file) On 2015/02/04 15:37:14, Wladimir Palant wrote: > On 2015/02/04 03:58:51, Felix H. Dahlke wrote: > > posixpath.join? > > No, we need a proper OS-specific path here, only the relative path needs to use > forward slashes. > > But now that you say it - relative path calculation below won't work correctly > on Windows, not even with posixpath. So far I've come up with the following > monster: > > def split_path(path): > path, filename = os.path.split(path) > return (split_path(path) if path else []) + [filename] > > ... > > relpath = posixpath.join(*split_path(os.path.relpath(path, root))) > > I think I better switch back to the original recursive approach. OK, fair enough. Considering that I didn't fully get the point behind relpath at first - how about renaming that one to "webpath" or something?
http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5657382461898752/run_... run_tests.py:33: path = os.path.join(dir, file) On 2015/02/04 18:20:25, Felix H. Dahlke wrote: > OK, fair enough. Considering that I didn't fully get the point behind relpath at > first - how about renaming that one to "webpath" or something? I don't think variable naming will make it any clearer. I've added a comment - and also made sure we use posixpath rather than os.path when manipulating these paths (for consistency).
LGTM
http://codereview.adblockplus.org/4895499043733504/diff/5681717746597888/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5681717746597888/run_... run_tests.py:65: data = "" Instead concatenating the whole string here, you might want to generate the response on the fly: http://flask.pocoo.org/docs/0.10/patterns/streaming/ Or at least I'd use at list, instead concatenating strings with the plus operator. I think you wouldn't even need to join that list but can just return it.
http://codereview.adblockplus.org/4895499043733504/diff/5681717746597888/run_... File run_tests.py (right): http://codereview.adblockplus.org/4895499043733504/diff/5681717746597888/run_... run_tests.py:65: data = "" On 2015/02/05 11:32:21, Sebastian Noack wrote: > Instead concatenating the whole string here, you might want to generate the > response on the fly: http://flask.pocoo.org/docs/0.10/patterns/streaming/ I think we are getting extremely nit-picky now, considering that the performance of this piece of code has no practical relevance whatsoever. But - fine with me, generators it is now.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
