Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1384243002:
Adds the ability to run pure Go unit tests in the Mojo test suite. (Closed)
|
Created:
5 years, 2 months ago by rudominer Modified:
5 years, 2 months ago CC:
azani, mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionAdds the ability to run pure Go unit tests in the Mojo test suite.
Go tests are run by changing into a directory containing a Go package and at least one file named *_test.go and invoking "go test". The go tool automatically builds the test binary in a temp directory and then runs it.
This CL just sets up the infrastructure for running these tests as part of the Mojo test suite. It adds a couple of dummy tests to check that the infrastructure works.
BUG=#461
R=ppi@chromium.org, viettrungluu@chromium.org, ppi
Committed: https://chromium.googlesource.com/external/mojo/+/5bb65cc1b4f88ee8b26300301a4eda118a134457
Patch Set 1 #
Total comments: 27
Patch Set 2 : Respond to code review comments. #
Total comments: 7
Patch Set 3 : Responds to code review commnets. #
Total comments: 11
Patch Set 4 : Specify pats to directories in a data file. Plus other fixes. #
Total comments: 4
Patch Set 5 : Further code review comment responses. #
Total comments: 3
Patch Set 6 : Refactors go.py and uses it to invoke the go tool. #Patch Set 7 : Adds dummy go tests to run. #
Total comments: 16
Patch Set 8 : Moved InvokeGo() to new script under mopy. #Patch Set 9 : Tweak some comments. #Patch Set 10 : Rebasing #
Messages
Total messages: 45 (21 generated)
rudominer@chromium.org changed reviewers: + ppi@chromium.org
Hi Przemek, Please take a look. thanks, Mitch
FYI, the first in a series of CLs for the new Mojom parser in Go is here https://codereview.chromium.org/1387893002/ and it is in line behind this CL. We would like to have some validation of the general technique for running pure Go tests as part of the Mojo suite before we start submitting the actual Go tests.
https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:121: # Go system tests Please end comment with ":" to match the others. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:125: # Pure Go unit tests Please end with ":". https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:131: AddEntry("Pure go unit tests", I'd just say "Go unit tests", as we have "Python unit tests" in the same spirit. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:10: I think the style guide wants two empty lines here. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:12: parser = argparse.ArgumentParser() Please add a desription string, ie. argparse.ArgumentParser(description="describe what it does") https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:13: parser.add_argument('--go_tool') I think we prefer "dash-only" arguments: --go-tool. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:13: parser.add_argument('--go_tool') If that is a required argument I'd make it positional, ie. remove the "--" from the name. this also removes the need for the assert later. Please add a "help" string explaining the usage. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:28: os.chdir(test_dir) There is an issue here that as "test_dir" is relative, it will work correctly only in the first run of the loop. We can a) move from subprocess.call to bare Popen, which accepts the cwd= argument (so that we don't need to modify the working directory of ourselves) b) ensure we restore the original cwd at the end of each loop run c) ensure the test_dirs are absolute paths As much as bare Popen is ugly to work with, I'd probably go with it. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:35: os.chdir(saved_cwd ) remove the space after "saved_cwd".
Thanks for the thorough review Przemek. ptal. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:121: # Go system tests On 2015/10/05 21:49:58, ppi wrote: > Please end comment with ":" to match the others. Done. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:125: # Pure Go unit tests On 2015/10/05 21:49:58, ppi wrote: > Please end with ":". Done. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:131: AddEntry("Pure go unit tests", On 2015/10/05 21:49:58, ppi wrote: > I'd just say "Go unit tests", as we have "Python unit tests" in the same spirit. Done. Note that I left the word "Pure" in the comment on line 125 because on line 116 the label "Go unit tests" is being used so I liked the label "Pure Go unit tests" to make it clear this is a sub-category. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:10: On 2015/10/05 21:49:58, ppi wrote: > I think the style guide wants two empty lines here. Done. But now the two empty lines are lines 12 and 13 below. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:12: parser = argparse.ArgumentParser() On 2015/10/05 21:49:58, ppi wrote: > Please add a desription string, ie. > argparse.ArgumentParser(description="describe what it does") Done. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:13: parser.add_argument('--go_tool') On 2015/10/05 21:49:58, ppi wrote: > I think we prefer "dash-only" arguments: --go-tool. Done. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:13: parser.add_argument('--go_tool') On 2015/10/05 21:49:58, ppi wrote: > I think we prefer "dash-only" arguments: --go-tool. Done. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:13: parser.add_argument('--go_tool') On 2015/10/05 21:49:58, ppi wrote: > If that is a required argument I'd make it positional, ie. remove the "--" from > the name. this also removes the need for the assert later. Please add a "help" > string explaining the usage. Done. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:28: os.chdir(test_dir) On 2015/10/05 21:49:58, ppi wrote: > There is an issue here that as "test_dir" is relative, it will work correctly > only in the first run of the loop. We can > > a) move from subprocess.call to bare Popen, which accepts the cwd= argument (so > that we don't need to modify the working directory of ourselves) > b) ensure we restore the original cwd at the end of each loop run > c) ensure the test_dirs are absolute paths > > As much as bare Popen is ugly to work with, I'd probably go with it. Thank you for catching this. I went with your option (c) if that is ok with you. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/run_pure_go_test... mojo/tools/run_pure_go_tests.py:35: os.chdir(saved_cwd ) On 2015/10/05 21:49:58, ppi wrote: > remove the space after "saved_cwd". Done.
viettrungluu@chromium.org changed reviewers: + viettrungluu@chromium.org
https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:126: if 'go_build_tool' in config.values: Apparently, in this file, we prefer double quotes. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:129: go_tool = os.path.join(Paths().src_root, 'third_party', 'go', 'tool', Should this path ever be taken?
Sorry I missed something and the build is failing. One moment....
https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:2: # Copyright (c) 2015 The Chromium Authors. All rights reserved. The current copyright does not have the "(c)". https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:24: saved_cwd = os.getcwd() Why do you bother doing this? https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:30: # TODO(rudominer) Add paths to go directories with tests. It seems like a bad idea to hard-code stuff in a script.
Patchset #3 (id:40001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
Przemek: I responded to your comments. ptal. Trung: I responded to most of your comments and asked for clarification about what you want instead of hard-coding the paths to the Go test directories. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:126: if 'go_build_tool' in config.values: On 2015/10/05 23:10:38, viettrungluu wrote: > Apparently, in this file, we prefer double quotes. Done. https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:129: go_tool = os.path.join(Paths().src_root, 'third_party', 'go', 'tool', On 2015/10/05 23:10:38, viettrungluu wrote: > Should this path ever be taken? The reason I added this is that on the try bots this function is not invoked via "mojob.py test" but rather this script's main is invoked from the command line and a json file is parsed in order to generate the config.values dictionary and in that case it appears that the go_build_tool property is not set. If you would prefer I could instead modify the json file so that the go_build_tool property is there. That will require me to figure out where this json file is. https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:2: # Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/10/05 23:16:59, viettrungluu wrote: > The current copyright does not have the "(c)". Done. https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:24: saved_cwd = os.getcwd() On 2015/10/05 23:16:59, viettrungluu wrote: > Why do you bother doing this? Good point. Removed. https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:30: # TODO(rudominer) Add paths to go directories with tests. On 2015/10/05 23:16:59, viettrungluu wrote: > It seems like a bad idea to hard-code stuff in a script. OK. What do you think about the following strategy: Add the new file mojo/tools/data/go_unittests and put the list in there using a format similar to the formats of the other files in that directory?
https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:129: go_tool = os.path.join(Paths().src_root, 'third_party', 'go', 'tool', On 2015/10/06 00:29:10, rudominer wrote: > On 2015/10/05 23:10:38, viettrungluu wrote: > > Should this path ever be taken? > The reason I added this is that on the try bots this function is not invoked via > "mojob.py test" but rather this script's main is invoked from the command line > and a json file is parsed in order to generate the config.values dictionary and > in that case it appears that the go_build_tool property is not set. > > If you would prefer I could instead modify the json file so that the > go_build_tool property is there. That will require me to figure out where this > json file is. That sounds like the right thing to do. https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/20001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:30: # TODO(rudominer) Add paths to go directories with tests. On 2015/10/06 00:29:10, rudominer wrote: > On 2015/10/05 23:16:59, viettrungluu wrote: > > It seems like a bad idea to hard-code stuff in a script. > > OK. What do you think about the following strategy: Add the new file > mojo/tools/data/go_unittests and put the list in there using a format similar to > the formats of the other files in that directory? SGTM https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/get_test_lis... File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/get_test_lis... mojo/tools/get_test_list.py:120: no blank line here https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/get_test_lis... mojo/tools/get_test_list.py:132: ["python", os.path.join("mojo", "tools", "run_pure_go_tests.py"), indent one more space https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:22: env['GOROOT'] = os.path.dirname(os.path.dirname(go_tool)) Maybe it should take the Go root as an argument instead (and infer the location of go)? Though I wonder if we shouldn't be invoking go via mojo/go/go.py (which may itself require updating) instead.
https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:30: # NOTE(rudominer) The paths should all be made absolute by including This will be better enforced by assert on os.path.isabs() later in the loop. I'd check this just before os.chdir, where it will be clear why we need the path to be absolute. https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:40: os.chdir(saved_cwd ) Need to drop this line too.
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
ptal https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:129: go_tool = os.path.join(Paths().src_root, 'third_party', 'go', 'tool', On 2015/10/06 01:46:29, viettrungluu wrote: > On 2015/10/06 00:29:10, rudominer wrote: > > On 2015/10/05 23:10:38, viettrungluu wrote: > > > Should this path ever be taken? > > The reason I added this is that on the try bots this function is not invoked > via > > "mojob.py test" but rather this script's main is invoked from the command line > > and a json file is parsed in order to generate the config.values dictionary > and > > in that case it appears that the go_build_tool property is not set. > > > > If you would prefer I could instead modify the json file so that the > > go_build_tool property is there. That will require me to figure out where this > > json file is. > > That sounds like the right thing to do. I discovered that the json file being consumed is generated by the function _GetTestConfig() in the bot recipe file which is located in the chrome infrastructure repo here: https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave.... Looking at that method it is not clear to me that it is the right place to put the path to the go binary. There is not other similar kinds of config data there. Also it seems wrong to me to put a path to something within the mojo repo in a file located in a different repo. For now I am leaving this as is and adding a note. At any rate adding the data to the recipe file would have to be done in a later CL. https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/get_test_lis... File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/get_test_lis... mojo/tools/get_test_list.py:120: On 2015/10/06 01:46:30, viettrungluu wrote: > no blank line here Done. https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/get_test_lis... mojo/tools/get_test_list.py:132: ["python", os.path.join("mojo", "tools", "run_pure_go_tests.py"), On 2015/10/06 01:46:30, viettrungluu wrote: > indent one more space Done. https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:30: # NOTE(rudominer) The paths should all be made absolute by including On 2015/10/06 17:57:47, ppi wrote: > This will be better enforced by assert on os.path.isabs() later in the loop. I'd > check this just before os.chdir, where it will be clear why we need the path to > be absolute. This is now organized differently. https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:40: os.chdir(saved_cwd ) On 2015/10/06 17:57:46, ppi wrote: > Need to drop this line too. Thanks. Done.
https://codereview.chromium.org/1384243002/diff/200001/mojo/tools/get_test_li... File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/200001/mojo/tools/get_test_li... mojo/tools/get_test_list.py:133: "linux_amd64", "bin", "go") I think we want two more spaces here. Or align with Paths() above. https://codereview.chromium.org/1384243002/diff/200001/mojo/tools/run_pure_go... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/200001/mojo/tools/run_pure_go... mojo/tools/run_pure_go_tests.py:33: src_root = Paths().src_root We'd still probably want to assert os.path.isabs(src_root).
ptal https://codereview.chromium.org/1384243002/diff/200001/mojo/tools/get_test_li... File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/200001/mojo/tools/get_test_li... mojo/tools/get_test_list.py:133: "linux_amd64", "bin", "go") On 2015/10/06 20:33:27, ppi wrote: > I think we want two more spaces here. Or align with Paths() above. Done. https://codereview.chromium.org/1384243002/diff/200001/mojo/tools/run_pure_go... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/200001/mojo/tools/run_pure_go... mojo/tools/run_pure_go_tests.py:33: src_root = Paths().src_root On 2015/10/06 20:33:27, ppi wrote: > We'd still probably want to assert os.path.isabs(src_root). Done.
lgtm
not lgtm https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:129: go_tool = os.path.join(Paths().src_root, 'third_party', 'go', 'tool', On 2015/10/06 20:28:53, rudominer wrote: > On 2015/10/06 01:46:29, viettrungluu wrote: > > On 2015/10/06 00:29:10, rudominer wrote: > > > On 2015/10/05 23:10:38, viettrungluu wrote: > > > > Should this path ever be taken? > > > The reason I added this is that on the try bots this function is not invoked > > via > > > "mojob.py test" but rather this script's main is invoked from the command > line > > > and a json file is parsed in order to generate the config.values dictionary > > and > > > in that case it appears that the go_build_tool property is not set. > > > > > > If you would prefer I could instead modify the json file so that the > > > go_build_tool property is there. That will require me to figure out where > this > > > json file is. > > > > That sounds like the right thing to do. > > I discovered that the json file being consumed is generated by the function > _GetTestConfig() in the bot recipe file which is located in the chrome > infrastructure repo here: > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave.... > Looking at that method it is not clear to me that it is the right place to put > the path to the go binary. There is not other similar kinds of config data > there. Also it seems wrong to me to put a path to something within the mojo repo > in a file located in a different repo. For now I am leaving this as is and > adding a note. Then the question is why it's in the config at all, rather than being derived from something in the config (which should be done in a common location). > At any rate adding the data to the recipe file would have to be > done in a later CL. You could add it now, with no harm.
https://codereview.chromium.org/1384243002/diff/220001/mojo/tools/get_test_li... File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/220001/mojo/tools/get_test_li... mojo/tools/get_test_list.py:132: go_tool = os.path.join(Paths().src_root, "third_party", "go", "tool", In particular, if the go_build_tool directory should be derived from other configuration parameters, then something should be added to Paths to figure that out. Moreover, using Paths() here is wrong. You should be using |paths|, which properly takes into account the config.
https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:22: env['GOROOT'] = os.path.dirname(os.path.dirname(go_tool)) On 2015/10/06 01:46:30, viettrungluu wrote: > Maybe it should take the Go root as an argument instead (and infer the location > of go)? > > Though I wonder if we shouldn't be invoking go via mojo/go/go.py (which may > itself require updating) instead. You didn't say anything about this.
Patchset #6 (id:240001) has been deleted
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #6 (id:320001) has been deleted
Patchset #6 (id:340001) has been deleted
Patchset #6 (id:360001) has been deleted
Patchset #6 (id:380001) has been deleted
Patchset #6 (id:400001) has been deleted
ptal https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/1/mojo/tools/get_test_list.py... mojo/tools/get_test_list.py:129: go_tool = os.path.join(Paths().src_root, 'third_party', 'go', 'tool', On 2015/10/06 21:03:44, viettrungluu wrote: > On 2015/10/06 20:28:53, rudominer wrote: > > On 2015/10/06 01:46:29, viettrungluu wrote: > > > On 2015/10/06 00:29:10, rudominer wrote: > > > > On 2015/10/05 23:10:38, viettrungluu wrote: > > > > > Should this path ever be taken? > > > > The reason I added this is that on the try bots this function is not > invoked > > > via > > > > "mojob.py test" but rather this script's main is invoked from the command > > line > > > > and a json file is parsed in order to generate the config.values > dictionary > > > and > > > > in that case it appears that the go_build_tool property is not set. > > > > > > > > If you would prefer I could instead modify the json file so that the > > > > go_build_tool property is there. That will require me to figure out where > > this > > > > json file is. > > > > > > That sounds like the right thing to do. > > > > I discovered that the json file being consumed is generated by the function > > _GetTestConfig() in the bot recipe file which is located in the chrome > > infrastructure repo here: > > > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave.... > > Looking at that method it is not clear to me that it is the right place to put > > the path to the go binary. There is not other similar kinds of config data > > there. Also it seems wrong to me to put a path to something within the mojo > repo > > in a file located in a different repo. For now I am leaving this as is and > > adding a note. > > Then the question is why it's in the config at all, rather than being derived > from something in the config (which should be done in a common location). Took your later suggestion and am now using paths. > > > At any rate adding the data to the recipe file would have to be > > done in a later CL. > > You could add it now, with no harm. https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/60001/mojo/tools/run_pure_go_... mojo/tools/run_pure_go_tests.py:22: env['GOROOT'] = os.path.dirname(os.path.dirname(go_tool)) On 2015/10/06 21:08:06, viettrungluu wrote: > On 2015/10/06 01:46:30, viettrungluu wrote: > > Maybe it should take the Go root as an argument instead (and infer the > location > > of go)? > > > > Though I wonder if we shouldn't be invoking go via mojo/go/go.py (which may > > itself require updating) instead. > > You didn't say anything about this. I'm sorry I missed this comment. I took your second suggestion and am now using go.py to invoke the go tool. https://codereview.chromium.org/1384243002/diff/220001/mojo/tools/get_test_li... File mojo/tools/get_test_list.py (right): https://codereview.chromium.org/1384243002/diff/220001/mojo/tools/get_test_li... mojo/tools/get_test_list.py:132: go_tool = os.path.join(Paths().src_root, "third_party", "go", "tool", On 2015/10/06 21:06:28, viettrungluu wrote: > In particular, if the go_build_tool directory should be derived from other > configuration parameters, then something should be added to Paths to figure that > out. > > Moreover, using Paths() here is wrong. You should be using |paths|, which > properly takes into account the config. Done. https://codereview.chromium.org/1384243002/diff/220001/mojo/tools/run_pure_go... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/220001/mojo/tools/run_pure_go... mojo/tools/run_pure_go_tests.py:41: return subprocess.call([go_tool, "test"], env=env) This return was a bug introduced in an earlier patchset.
Patchset #7 (id:440001) has been deleted
I added a couple of dummy tests in order to test the new testing infrastructure. ptal.
https://codereview.chromium.org/1384243002/diff/460001/mojo/go/go.py File mojo/go/go.py (right): https://codereview.chromium.org/1384243002/diff/460001/mojo/go/go.py#newcode64 mojo/go/go.py:64: def InvokeGo(go_tool, go_options, work_dir=None, src_root=None, Probably this should be put under mojo/tools/mopy: It's bad/unexpected to have something undir mojo/tools depend on something outside. https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/data/gotests File mojo/tools/data/gotests (right): https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/data/gotest... mojo/tools/data/gotests:9: ['mojom', 'mojom_parser', 'lexer'], Representing a path as a list is pretty user-unfriendly. I suggest just using strings (separating components using '/'). https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/mopy/paths.py File mojo/tools/mopy/paths.py (right): https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/mopy/paths.... mojo/tools/mopy/paths.py:23: if config and config.target_os == Config.OS_LINUX: Probably nest these under a single |if config:|. https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/mopy/paths.... mojo/tools/mopy/paths.py:25: "go", "tool", "linux_amd64", "bin", "go") Probably you should also check that the target arch is x64 also. https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/mopy/paths.... mojo/tools/mopy/paths.py:26: if config and config.target_os == Config.OS_ANDROID: elif https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/run_pure_go... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/run_pure_go... mojo/tools/run_pure_go_tests.py:5: Needs a file-level docstring. https://codereview.chromium.org/1384243002/diff/460001/mojom/mojom_parser/lex... File mojom/mojom_parser/lexer/lexer_test.go (right): https://codereview.chromium.org/1384243002/diff/460001/mojom/mojom_parser/lex... mojom/mojom_parser/lexer/lexer_test.go:1: package lexer nit: Needs copyright header. https://codereview.chromium.org/1384243002/diff/460001/mojom/mojom_parser/par... File mojom/mojom_parser/parser/parser_test.go (right): https://codereview.chromium.org/1384243002/diff/460001/mojom/mojom_parser/par... mojom/mojom_parser/parser/parser_test.go:1: package parser "
Patchset #8 (id:480001) has been deleted
Patchset #8 (id:500001) has been deleted
ptal. I'm not sure what's up with the test failures but I'm pretty sure it is unrelated to this CL. https://codereview.chromium.org/1384243002/diff/460001/mojo/go/go.py File mojo/go/go.py (right): https://codereview.chromium.org/1384243002/diff/460001/mojo/go/go.py#newcode64 mojo/go/go.py:64: def InvokeGo(go_tool, go_options, work_dir=None, src_root=None, On 2015/10/09 21:39:49, viettrungluu wrote: > Probably this should be put under mojo/tools/mopy: It's bad/unexpected to have > something undir mojo/tools depend on something outside. Done. https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/data/gotests File mojo/tools/data/gotests (right): https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/data/gotest... mojo/tools/data/gotests:9: ['mojom', 'mojom_parser', 'lexer'], On 2015/10/09 21:39:49, viettrungluu wrote: > Representing a path as a list is pretty user-unfriendly. I suggest just using > strings (separating components using '/'). Done. https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/mopy/paths.py File mojo/tools/mopy/paths.py (right): https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/mopy/paths.... mojo/tools/mopy/paths.py:23: if config and config.target_os == Config.OS_LINUX: On 2015/10/09 21:39:49, viettrungluu wrote: > Probably nest these under a single |if config:|. Done. https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/mopy/paths.... mojo/tools/mopy/paths.py:25: "go", "tool", "linux_amd64", "bin", "go") On 2015/10/09 21:39:49, viettrungluu wrote: > Probably you should also check that the target arch is x64 also. Done. https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/mopy/paths.... mojo/tools/mopy/paths.py:26: if config and config.target_os == Config.OS_ANDROID: On 2015/10/09 21:39:49, viettrungluu wrote: > elif Done. https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/run_pure_go... File mojo/tools/run_pure_go_tests.py (right): https://codereview.chromium.org/1384243002/diff/460001/mojo/tools/run_pure_go... mojo/tools/run_pure_go_tests.py:5: On 2015/10/09 21:39:49, viettrungluu wrote: > Needs a file-level docstring. Done. https://codereview.chromium.org/1384243002/diff/460001/mojom/mojom_parser/lex... File mojom/mojom_parser/lexer/lexer_test.go (right): https://codereview.chromium.org/1384243002/diff/460001/mojom/mojom_parser/lex... mojom/mojom_parser/lexer/lexer_test.go:1: package lexer On 2015/10/09 21:39:50, viettrungluu wrote: > nit: Needs copyright header. Done. https://codereview.chromium.org/1384243002/diff/460001/mojom/mojom_parser/par... File mojom/mojom_parser/parser/parser_test.go (right): https://codereview.chromium.org/1384243002/diff/460001/mojom/mojom_parser/par... mojom/mojom_parser/parser/parser_test.go:1: package parser On 2015/10/09 21:39:50, viettrungluu wrote: > " Done.
Tests are green now except one device status check for Android.
Thanks, lgtm.
Message was sent while issue was closed.
Committed patchset #10 (id:560001) manually as 5bb65cc1b4f88ee8b26300301a4eda118a134457 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
