Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7458012:
Create a "no compile" drivers script in python to unittest compile time asserts. (Closed)
|
Created:
9 years, 5 months ago by awong Modified:
9 years, 3 months ago CC:
chromium-reviews, brettw-cc_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreate a "no compile" drivers script in python to unittest compile time asserts.
BUG=87341
TEST=enable some of the existing no-compile tests and run on try bots.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100564
Patch Set 1 #Patch Set 2 : Stub-out nocompile.gypi as informal design proposal. #Patch Set 3 : working no-compile tests. #Patch Set 4 : increase timeouts #
Total comments: 90
Patch Set 5 : twiddle with timeouts more. #Patch Set 6 : stupid errors #Patch Set 7 : Address Ami's comments and don't busyloop. #
Total comments: 27
Patch Set 8 : Address Ami's comments, add more stats, and fill in unittests. #
Total comments: 4
Patch Set 9 : address last 2 comments #
Messages
Total messages: 17 (0 generated)
Hey guys, Here's a proposal for a negative compile test driver. The idea would be to hook this into a python unittest, and then grab the correct gcc location and cflags location from gyp. The "test_configs" would be specified in a big gyp variable. Thoughts? This is based off the google3 CppNCTest used by Google test.
Do you have locations where you plan on using this?
Yes. Bind-unttests.cc has a bunch of compiled out "Don't allow this to compile " lines. Look there for a frame of reference. Note...I'm still not sure how to hook his into gyp yet. On Jul 27, 2011 8:05 PM, <mark@chromium.org> wrote: > Do you have locations where you plan on using this? > > http://codereview.chromium.org/7458012/
That was my next question…
On Wed, Jul 27, 2011 at 8:13 PM, <mark@chromium.org> wrote: > That was my next question… Yeah...one could imagine a no_compile target. One could also imagine a pyunit test that loads this as a module (would need to work this slightly to be module friendly)...just make sure no threads are spawned. -Albert
Added a nocompile.gypi with outlines how I think this could get integrated into the build.
PTAL
Leaving Dubai now. Will review when I land in SF. On Aug 31, 2011 6:33 AM, <ajwong@chromium.org> wrote: > PTAL > > http://codereview.chromium.org/7458012/
Ami's offered to look at the python script. Will try to find a good Gyp reviewer. Maybe will land this in 2 stages.
Lots of nits. Overall I like it! http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py File tools/nocompile_driver.py (right): http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:10: invoke gcc on a source file and assert that compilation fails. Will there be a sites/wiki page about this feature? If so point to it from here. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:20: import time alphabetize this list http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:23: class ArgumentValidationException(Exception): Is this a NoCompileSyntaxError? Maybe a docstring would help me understand the name better. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:23: class ArgumentValidationException(Exception): Don't all global names that aren't meant to be peeked at from outside this file need to have a leading-underscore name? (or do you not care about that b/c this is a standalone script?) http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:31: # #ifdef TEST_NAME_OF_TEST // [r'expected output'] I'm bothered by TEST appearing twice in each of these examples, and by your acceptance of TEST anywhere in the conditional; I fear this will be insufficiently specific (catch unintended lines). But I guess this will only be applied to .nc files anyway, so maybe that's OK? WDYT of using NCTEST instead of TEST and insisting it be immediately preceded by \s or \(? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:40: # Used the removed the defined() preprocesor predicate if the test uses english the comment up. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:45: # Should be used to post-process the results above. s/above/found by TEST_CONFIG_RE/ http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:53: EXTRACT_EXPECTATION_RE = re.compile(r'//\s*(\[.*\])') Any reason not to make this part of TEST_CONFIG_RE? If you keep it separate, it can go inside ParseExpectation, no? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:71: def ValidateInput(parallelism, sourcefile_path, cflags, resultfile_path): docstring here and below please. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:72: if parallelism < 1: If it was me I'd replace lines 72-86 with: assert parallelism >= 1 assert type(sourcefile_path) is str assert type(cflags) is str assert type(resultfile_path) is str (ditto for the rest of the raises below; there's an awful lot of vertical space going to sanity checking here) http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:105: if match is None: do you use "is None" over "not" intentionally? (note I would just assert match, but that's just me) http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:120: re.compile(regex_str) unreachable http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:122: return expectation doco semantics of empty list. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:134: sourcefile_path: A string with path to the source file. "with"? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:140: suite_name: 'SOURCE_PATH_NAME' SourcePathName? (what is this for? gtest mockery? Might say a word or two) http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:143: The compiled regexps in expectations define the valid outputs of the s/expectations/|expectations|/ http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:147: compiler output and just check for failed compilation. If the expectations ditto (and drop "the") http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:148: is actually None, then this specifies a compiler sanity check test, which this is the first time in the file you mention "sanity" tests. Since this is emitted regardless of source file wdyt of removing it from this function and adding it in main? That way you can also drop this description. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:149: should a SUCCESSFUL compilation. english http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:151: sourcefile = open(sourcefile_path, 'r') or die? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:154: words = os.path.splitext('bind_unittest.nc')[0].split('_') 'bind_unittest.nc' whaaa? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:158: # Start with at least the compiler sanity test. IMO this needs a justifying comment (IIUC you do this to have evidence in the test output that the nc test got looked at at all). http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:165: if match_result: if you reverse the test you get to early-continue and de-indent the rest of the function. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:175: expectations = ParseExpecation(groups[1]) temporary isn't used anywhere but the dict below so could in-line the call to ParseExpectation if you want. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:185: """Perform one negative compile test. s/Perform/Start/ http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:189: cflags: A string with all the CFLAGS to give to gcc. doco passing through shlex http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:191: for a description of the config format. doco return value http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:215: """Interprets and logs the result of a test started by StartTest() There isn't much interpreting going on (here and in FailTest). http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:218: resultfile: File object for .cc file that results are written to. doco |test| http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:221: resultfile.write('\n') throw into previous write()? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:228: resultfile: File object for .cc file that results are written to. doco other args http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:280: matched = True PassTest & return? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:286: 'Expecatations [%s] did not match output.' % expectation_str, typo http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:290: # No failures. Success! If you take my suggestion at l.280 above, this can be removed. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:294: def DrainExecutingTasks(resultfile, executing_tests): DrainAtLeastOneExecutingTask? I don't like either name. Ideas? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:295: """Blocks until one task is removed from executing_tests. s/one/at least one/ http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:304: executing_tests: Array of currently executing tests from StartTest(). doco return value http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:332: if len(sys.argv) < 3: s/</!=/ http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:336: pass why the pass? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:338: # Force us to the "C" locale so that compiler doesn't localize its output. s/that/the/ http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:338: # Force us to the "C" locale so that compiler doesn't localize its output. This is strange. Is this something that gyp does for compile commands but not for "actions"? is that a gyp bug? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:351: print 'resultfile: %s' % resultfile_path Drop these?
Thanks Ami. I guess I won't have to review this now.
TAL s'il vous plaît. I think I got rid of the busy loop. But the compile failures take forever to complete running. Ick. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py File tools/nocompile_driver.py (right): http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:10: invoke gcc on a source file and assert that compilation fails. On 2011/08/31 21:50:18, Ami Fischman wrote: > Will there be a sites/wiki page about this feature? If so point to it from > here. Created a skeleton. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:20: import time On 2011/08/31 21:50:18, Ami Fischman wrote: > alphabetize this list Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:23: class ArgumentValidationException(Exception): On 2011/08/31 21:50:18, Ami Fischman wrote: > Don't all global names that aren't meant to be peeked at from outside this file > need to have a leading-underscore name? > > (or do you not care about that b/c this is a standalone script?) It's a standalone script so making everything _ prefixed seems overkill. Also, it's an exception which means that if someone loaded this as a module, they'd see this error. So I think it should not be prefixed with an _. See http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:23: class ArgumentValidationException(Exception): On 2011/08/31 21:50:18, Ami Fischman wrote: > Is this a NoCompileSyntaxError? > Maybe a docstring would help me understand the name better. Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:31: # #ifdef TEST_NAME_OF_TEST // [r'expected output'] On 2011/08/31 21:50:18, Ami Fischman wrote: > I'm bothered by TEST appearing twice in each of these examples, and by your > acceptance of TEST anywhere in the conditional; I fear this will be > insufficiently specific (catch unintended lines). But I guess this will only be > applied to .nc files anyway, so maybe that's OK? > WDYT of using NCTEST instead of TEST and insisting it be immediately preceded by > \s or \(? I like making it more specific with NCTEST. However, I also want to catch the DISABLED_ prefix, and don't feel like making the regex any more complicated. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:40: # Used the removed the defined() preprocesor predicate if the test uses On 2011/08/31 21:50:18, Ami Fischman wrote: > english the comment up. Tried to rewrite. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:45: # Should be used to post-process the results above. On 2011/08/31 21:50:18, Ami Fischman wrote: > s/above/found by TEST_CONFIG_RE/ Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:53: EXTRACT_EXPECTATION_RE = re.compile(r'//\s*(\[.*\])') On 2011/08/31 21:50:18, Ami Fischman wrote: > Any reason not to make this part of TEST_CONFIG_RE? Seemed like too much logic in one regex. > If you keep it separate, it can go inside ParseExpectation, no? Felt nicer to keep all the regexps grouped, and to avoid multiply compiling the pattern. Really, none of these need to be outside their function. If you still think I should move it, I'll do it. But feels more clear up here to me. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:71: def ValidateInput(parallelism, sourcefile_path, cflags, resultfile_path): On 2011/08/31 21:50:18, Ami Fischman wrote: > docstring here and below please. Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:72: if parallelism < 1: On 2011/08/31 21:50:18, Ami Fischman wrote: > If it was me I'd replace lines 72-86 with: > assert parallelism >= 1 > assert type(sourcefile_path) is str > assert type(cflags) is str > assert type(resultfile_path) is str > > (ditto for the rest of the raises below; there's an awful lot of vertical space > going to sanity checking here) Good point. Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:105: if match is None: On 2011/08/31 21:50:18, Ami Fischman wrote: > do you use "is None" over "not" intentionally? > (note I would just assert match, but that's just me) Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:120: re.compile(regex_str) On 2011/08/31 21:50:18, Ami Fischman wrote: > unreachable Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:122: return expectation On 2011/08/31 21:50:18, Ami Fischman wrote: > doco semantics of empty list. Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:134: sourcefile_path: A string with path to the source file. On 2011/08/31 21:50:18, Ami Fischman wrote: > "with"? "containing"? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:140: suite_name: 'SOURCE_PATH_NAME' On 2011/08/31 21:50:18, Ami Fischman wrote: > SourcePathName? > > (what is this for? gtest mockery? Might say a word or two) Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:143: The compiled regexps in expectations define the valid outputs of the On 2011/08/31 21:50:18, Ami Fischman wrote: > s/expectations/|expectations|/ Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:147: compiler output and just check for failed compilation. If the expectations On 2011/08/31 21:50:18, Ami Fischman wrote: > ditto (and drop "the") Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:149: should a SUCCESSFUL compilation. On 2011/08/31 21:50:18, Ami Fischman wrote: > english Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:151: sourcefile = open(sourcefile_path, 'r') On 2011/08/31 21:50:18, Ami Fischman wrote: > or die? I thought python just raises on errors... http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:154: words = os.path.splitext('bind_unittest.nc')[0].split('_') On 2011/08/31 21:50:18, Ami Fischman wrote: > 'bind_unittest.nc' whaaa? oops. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:158: # Start with at least the compiler sanity test. On 2011/08/31 21:50:18, Ami Fischman wrote: > IMO this needs a justifying comment (IIUC you do this to have evidence in the > test output that the nc test got looked at at all). Justification added. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:165: if match_result: On 2011/08/31 21:50:18, Ami Fischman wrote: > if you reverse the test you get to early-continue and de-indent the rest of the > function. Good call. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:175: expectations = ParseExpecation(groups[1]) On 2011/08/31 21:50:18, Ami Fischman wrote: > temporary isn't used anywhere but the dict below so could in-line the call to > ParseExpectation if you want. inlined :) http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:185: """Perform one negative compile test. On 2011/08/31 21:50:18, Ami Fischman wrote: > s/Perform/Start/ Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:189: cflags: A string with all the CFLAGS to give to gcc. On 2011/08/31 21:50:18, Ami Fischman wrote: > doco passing through shlex Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:191: for a description of the config format. On 2011/08/31 21:50:18, Ami Fischman wrote: > doco return value Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:215: """Interprets and logs the result of a test started by StartTest() On 2011/08/31 21:50:19, Ami Fischman wrote: > There isn't much interpreting going on (here and in FailTest). Comment fixed. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:218: resultfile: File object for .cc file that results are written to. On 2011/08/31 21:50:19, Ami Fischman wrote: > doco |test| Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:221: resultfile.write('\n') On 2011/08/31 21:50:19, Ami Fischman wrote: > throw into previous write()? Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:228: resultfile: File object for .cc file that results are written to. On 2011/08/31 21:50:19, Ami Fischman wrote: > doco other args Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:280: matched = True On 2011/08/31 21:50:19, Ami Fischman wrote: > PassTest & return? Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:286: 'Expecatations [%s] did not match output.' % expectation_str, On 2011/08/31 21:50:19, Ami Fischman wrote: > typo Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:290: # No failures. Success! On 2011/08/31 21:50:19, Ami Fischman wrote: > If you take my suggestion at l.280 above, this can be removed. Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:294: def DrainExecutingTasks(resultfile, executing_tests): On 2011/08/31 21:50:19, Ami Fischman wrote: > DrainAtLeastOneExecutingTask? > I don't like either name. Ideas? I don't have any good names either. This function just kinda sucks. CompleteAtLeastOneTest? http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:295: """Blocks until one task is removed from executing_tests. On 2011/08/31 21:50:19, Ami Fischman wrote: > s/one/at least one/ Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:304: executing_tests: Array of currently executing tests from StartTest(). On 2011/08/31 21:50:19, Ami Fischman wrote: > doco return value Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:332: if len(sys.argv) < 3: On 2011/08/31 21:50:19, Ami Fischman wrote: > s/</!=/ Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:336: pass On 2011/08/31 21:50:19, Ami Fischman wrote: > why the pass? removed. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:338: # Force us to the "C" locale so that compiler doesn't localize its output. On 2011/08/31 21:50:19, Ami Fischman wrote: > s/that/the/ Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:338: # Force us to the "C" locale so that compiler doesn't localize its output. On 2011/08/31 21:50:19, Ami Fischman wrote: > This is strange. Is this something that gyp does for compile commands but not > for "actions"? is that a gyp bug? No...it's just gcc localizing its output. If you're in something like LANG=en_US.utf8, it will get fancy and use smart quotes. Keeping things in LANG="C" keeps things in ASCII. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:351: print 'resultfile: %s' % resultfile_path On 2011/08/31 21:50:19, Ami Fischman wrote: > Drop these? dropped.
http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py File tools/nocompile_driver.py (right): http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:23: class ArgumentValidationException(Exception): > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... I'm not sure what you're pointing out there; the only relevant bit I see (quoting out of context b/c it's talking about main) is: "Even a file meant to be used as a script should be importable" I don't care strongly about this. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:53: EXTRACT_EXPECTATION_RE = re.compile(r'//\s*(\[.*\])') On 2011/09/08 02:46:16, awong wrote: > On 2011/08/31 21:50:18, Ami Fischman wrote: > > Any reason not to make this part of TEST_CONFIG_RE? > > Seemed like too much logic in one regex. > > > If you keep it separate, it can go inside ParseExpectation, no? > > Felt nicer to keep all the regexps grouped, and to avoid multiply compiling the > pattern. Really, none of these need to be outside their function. > > If you still think I should move it, I'll do it. But feels more clear up here > to me. Fine to leave as-is; I'm just silently judging your aesthetic sense. Yes, this is how I judge silently. You wouldn't want to hear the non-silent version... http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:134: sourcefile_path: A string with path to the source file. On 2011/09/08 02:46:16, awong wrote: > On 2011/08/31 21:50:18, Ami Fischman wrote: > > "with"? > > "containing"? "The path to the source file"? (append "in string form" if you think that's relevant) http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:148: is actually None, then this specifies a compiler sanity check test, which On 2011/08/31 21:50:18, Ami Fischman wrote: > this is the first time in the file you mention "sanity" tests. Since this is > emitted regardless of source file wdyt of removing it from this function and > adding it in main? That way you can also drop this description. Missed this or didn't like it? http://codereview.chromium.org/7458012/diff/24001/build/nocompile.gypi File build/nocompile.gypi (right): http://codereview.chromium.org/7458012/diff/24001/build/nocompile.gypi#newcode10 build/nocompile.gypi:10: # https://sites.google.com/a/chromium.org/dev/developers/testing/no-compile-tests http://dev.chromium.org/developers/testing/no-compile-test instead? http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py File tools/nocompile_driver.py (right): http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:13: https://sites.google.com/a/chromium.org/dev/developers/testing/no-compile-tests Not found for me; also would be nice to use the dev.chromium.org hostname. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:87: NCTEST_TERMINATE_TIMEOUT_SEC = 60 Channeling phajdan.jr, is there a standard timeout that'd be appropriate to import into this module and use? I don't know... http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:87: NCTEST_TERMINATE_TIMEOUT_SEC = 60 IWBN for that 60 to be significantly lower. I see you have a side-thread going about that. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:113: raise ConfigurationSyntaxError('Test must specify expected output.') I'm a bit surprised you still have ConfigurationSyntaxError anywhere (as opposed to simply asserting). http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:205: split by shelex so becareful with escaping. s/becareful/be careful/ http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:368: A tuple with a set of tests that have finished. "tuple with a set" sounds like a set is involved, but none is. (general comment: you use a comment style emphasizing the type of the variable over its contents; it's almost like you expect the reader to be a C compiler ;)) http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:378: # We timed out on all running tests. Assume this whole thing is hung. indent http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:380: raise WatchdogException('Busy looping for too long. Aborting no compile ' s/no compile/no-compile/ (b/c otherwise the "no" binds to "Aborting" for me) http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:380: raise WatchdogException('Busy looping for too long. Aborting no compile ' Where does WatchdogException come from? http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:441: executing_tests[test['name']] = test Any point in making assertions about uniqueness of test names? http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:446: just_finished = CompleteAtLeastOneTest(resultfile, executing_tests) Possible to avoid the duplication between l.446-449 and l.431-435? http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:449: del executing_tests[test['name']] If this was in CompleteAtLeastOneTest it'd be only half the duplication to worry about here...
PTAL Still trying to work out slow compile issue. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py File tools/nocompile_driver.py (right): http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:23: class ArgumentValidationException(Exception): On 2011/09/08 18:30:20, Ami Fischman wrote: > > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... > > I'm not sure what you're pointing out there; the only relevant bit I see > (quoting out of context b/c it's talking about main) is: > "Even a file meant to be used as a script should be importable" > > I don't care strongly about this. The naming box at the bottom shows that exceptions don't get prefixed with _ even if they are private... But yes, I don't care. And I'm going to remove this in favor of asserts. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:134: sourcefile_path: A string with path to the source file. On 2011/09/08 18:30:20, Ami Fischman wrote: > On 2011/09/08 02:46:16, awong wrote: > > On 2011/08/31 21:50:18, Ami Fischman wrote: > > > "with"? > > > > "containing"? > > "The path to the source file"? > (append "in string form" if you think that's relevant) Done. http://codereview.chromium.org/7458012/diff/16001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:148: is actually None, then this specifies a compiler sanity check test, which On 2011/09/08 18:30:20, Ami Fischman wrote: > On 2011/08/31 21:50:18, Ami Fischman wrote: > > this is the first time in the file you mention "sanity" tests. Since this is > > emitted regardless of source file wdyt of removing it from this function and > > adding it in main? That way you can also drop this description. > > Missed this or didn't like it? Missed, but I'm not sure I like. :) I'd rather only have 1 location generates these configuration structures. http://codereview.chromium.org/7458012/diff/24001/build/nocompile.gypi File build/nocompile.gypi (right): http://codereview.chromium.org/7458012/diff/24001/build/nocompile.gypi#newcode10 build/nocompile.gypi:10: # https://sites.google.com/a/chromium.org/dev/developers/testing/no-compile-tests On 2011/09/08 18:30:20, Ami Fischman wrote: > http://dev.chromium.org/developers/testing/no-compile-test instead? Done. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py File tools/nocompile_driver.py (right): http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:13: https://sites.google.com/a/chromium.org/dev/developers/testing/no-compile-tests On 2011/09/08 18:30:20, Ami Fischman wrote: > Not found for me; also would be nice to use the http://dev.chromium.org hostname. Strange...I'm able to see it. I've updated with the dev.chromium.org version. Try it again? http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:87: NCTEST_TERMINATE_TIMEOUT_SEC = 60 On 2011/09/08 18:30:20, Ami Fischman wrote: > Channeling phajdan.jr, is there a standard timeout that'd be appropriate to > import into this module and use? I don't know... I have no clue what timeout would make sense. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:87: NCTEST_TERMINATE_TIMEOUT_SEC = 60 On 2011/09/08 18:30:20, Ami Fischman wrote: > IWBN for that 60 to be significantly lower. I see you have a side-thread going > about that. Yes...on my z600, it's pretty reliably < 2 seconds.. Originally this was 5, which I think is reasonable. However, the try bots made this go all crazy. Still trying to debug. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:113: raise ConfigurationSyntaxError('Test must specify expected output.') On 2011/09/08 18:30:20, Ami Fischman wrote: > I'm a bit surprised you still have ConfigurationSyntaxError anywhere (as opposed > to simply asserting). All gone. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:205: split by shelex so becareful with escaping. On 2011/09/08 18:30:20, Ami Fischman wrote: > s/becareful/be careful/ Done. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:368: A tuple with a set of tests that have finished. On 2011/09/08 18:30:20, Ami Fischman wrote: > "tuple with a set" sounds like a set is involved, but none is. > (general comment: you use a comment style emphasizing the type of the variable > over its contents; it's almost like you expect the reader to be a C compiler ;)) Yeah yeah...dynamic languages make me uncomfortable. :( Fixed the comment. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:378: # We timed out on all running tests. Assume this whole thing is hung. On 2011/09/08 18:30:20, Ami Fischman wrote: > indent deleted. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:380: raise WatchdogException('Busy looping for too long. Aborting no compile ' On 2011/09/08 18:30:20, Ami Fischman wrote: > s/no compile/no-compile/ > (b/c otherwise the "no" binds to "Aborting" for me) deleted. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:380: raise WatchdogException('Busy looping for too long. Aborting no compile ' On 2011/09/08 18:30:20, Ami Fischman wrote: > Where does WatchdogException come from? deleted http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:441: executing_tests[test['name']] = test On 2011/09/08 18:30:20, Ami Fischman wrote: > Any point in making assertions about uniqueness of test names? Done. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:446: just_finished = CompleteAtLeastOneTest(resultfile, executing_tests) On 2011/09/08 18:30:20, Ami Fischman wrote: > Possible to avoid the duplication between l.446-449 and l.431-435? Not that I can think of. But I took your other suggestion and reduced it down to 1 line. http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:449: del executing_tests[test['name']] On 2011/09/08 18:30:20, Ami Fischman wrote: > If this was in CompleteAtLeastOneTest it'd be only half the duplication to worry > about here... Done.
LGTM A pleasure doing business! http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py File tools/nocompile_driver.py (right): http://codereview.chromium.org/7458012/diff/24001/tools/nocompile_driver.py#n... tools/nocompile_driver.py:13: https://sites.google.com/a/chromium.org/dev/developers/testing/no-compile-tests On 2011/09/09 01:01:43, awong wrote: > On 2011/09/08 18:30:20, Ami Fischman wrote: > > Not found for me; also would be nice to use the http://dev.chromium.org > hostname. > > Strange...I'm able to see it. I've updated with the http://dev.chromium.org version. > Try it again? WFM now. http://codereview.chromium.org/7458012/diff/34007/base/bind_unittest.nc File base/bind_unittest.nc (right): http://codereview.chromium.org/7458012/diff/34007/base/bind_unittest.nc#newco... base/bind_unittest.nc:10: // Keep all the test declarations outside of an anonymous namespace to avoid Umm, what? http://codereview.chromium.org/7458012/diff/34007/base/bind_unittest.nc#newco... base/bind_unittest.nc:12: const int kParentValue = 1; static is implied so I enjoy putting it in explicitly. Up to you.
http://codereview.chromium.org/7458012/diff/34007/base/bind_unittest.nc File base/bind_unittest.nc (right): http://codereview.chromium.org/7458012/diff/34007/base/bind_unittest.nc#newco... base/bind_unittest.nc:10: // Keep all the test declarations outside of an anonymous namespace to avoid On 2011/09/09 17:17:12, Ami Fischman wrote: > Umm, what? Clarified. http://codereview.chromium.org/7458012/diff/34007/base/bind_unittest.nc#newco... base/bind_unittest.nc:12: const int kParentValue = 1; On 2011/09/09 17:17:12, Ami Fischman wrote: > static is implied so I enjoy putting it in explicitly. > Up to you. Done.
gypi LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
