Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
639713002:
mojo: Add python bindings tests to mojob.sh (Closed)
|
Created:
6 years, 2 months ago by qsr Modified:
6 years, 2 months ago Reviewers:
viettrungluu CC:
chromium-reviews, 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://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionmojo: Add python bindings tests to mojob.sh
R=viettrungluu@chromium.org
Committed: https://crrev.com/e3b64d0d60ce004ba7976cbe5b55548475a81ea2
Cr-Commit-Position: refs/heads/master@{#299124}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Follow review #Patch Set 3 : Check on library existence instead of config of the test. #Messages
Total messages: 13 (1 generated)
https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh File mojo/tools/mojob.sh (right): https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode70 mojo/tools/mojob.sh:70: if [ "$COMPONENT" = "static" ]; then This is rather dubious, since the component flags typically only affect gyp/gypall, so one wouldn't usually specify it when running tests. https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode72 mojo/tools/mojob.sh:72: "--build-dir=out/$1" || exit 1 And if it runs things out of a particular build directory, it should tell you which one. https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode245 mojo/tools/mojob.sh:245: exit 0 Is there a reason for adding this?
https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh File mojo/tools/mojob.sh (right): https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode70 mojo/tools/mojob.sh:70: if [ "$COMPONENT" = "static" ]; then On 2014/10/08 16:06:32, viettrungluu wrote: > This is rather dubious, since the component flags typically only affect > gyp/gypall, so one wouldn't usually specify it when running tests. Unfortunately, I do not have a good solution for this :( The generated python modules do not work in component builds... Do you have a suggestion? https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode72 mojo/tools/mojob.sh:72: "--build-dir=out/$1" || exit 1 On 2014/10/08 16:06:33, viettrungluu wrote: > And if it runs things out of a particular build directory, it should tell you > which one. Done. https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode245 mojo/tools/mojob.sh:245: exit 0 On 2014/10/08 16:06:32, viettrungluu wrote: > Is there a reason for adding this? The script exit with 1 when called for debug. My guess is that it doesn't like to have the last called instruction be a false ||
gentle ping?
On 2014/10/08 16:16:08, qsr wrote: > https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh > File mojo/tools/mojob.sh (right): > > https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode70 > mojo/tools/mojob.sh:70: if [ "$COMPONENT" = "static" ]; then > On 2014/10/08 16:06:32, viettrungluu wrote: > > This is rather dubious, since the component flags typically only affect > > gyp/gypall, so one wouldn't usually specify it when running tests. > > Unfortunately, I do not have a good solution for this :( The generated python > modules do not work in component builds... Are they still generated? Is there a way to tell if the out directory has a component build or not? (We may be getting rid of the component build soon, so maybe add a TODO for yourself, at least.) > > Do you have a suggestion? > > https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode72 > mojo/tools/mojob.sh:72: "--build-dir=out/$1" || exit 1 > On 2014/10/08 16:06:33, viettrungluu wrote: > > And if it runs things out of a particular build directory, it should tell you > > which one. > > Done. > > https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode245 > mojo/tools/mojob.sh:245: exit 0 > On 2014/10/08 16:06:32, viettrungluu wrote: > > Is there a reason for adding this? > > The script exit with 1 when called for debug. > > My guess is that it doesn't like to have the last called instruction be a false > ||
https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh File mojo/tools/mojob.sh (right): https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode245 mojo/tools/mojob.sh:245: exit 0 On 2014/10/08 16:16:08, qsr wrote: > On 2014/10/08 16:06:32, viettrungluu wrote: > > Is there a reason for adding this? > > The script exit with 1 when called for debug. Sorry, what was the case (e.g., command-line) when it exited with 1? > > My guess is that it doesn't like to have the last called instruction be a false > || But which one would that be (given that it's always "|| exit 1"?
On 2014/10/09 22:49:52, viettrungluu wrote: > https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh > File mojo/tools/mojob.sh (right): > > https://codereview.chromium.org/639713002/diff/1/mojo/tools/mojob.sh#newcode245 > mojo/tools/mojob.sh:245: exit 0 > On 2014/10/08 16:16:08, qsr wrote: > > On 2014/10/08 16:06:32, viettrungluu wrote: > > > Is there a reason for adding this? > > > > The script exit with 1 when called for debug. > > Sorry, what was the case (e.g., command-line) when it exited with 1? mojo/tools/mojob.sh --debug build test > > > > > My guess is that it doesn't like to have the last called instruction be a > false > > || > > But which one would that be (given that it's always "|| exit 1"? If you use --debug, the last shell command will be: should_do_Release && do_unittests Release and this will not exit, but will make the shell exit with 1.
> Are they still generated? Is there a way to tell if the out directory has a > component build or not? > > (We may be getting rid of the component build soon, so maybe add a TODO for > yourself, at least.) They are not generated in a component build, so checking on the module instead of the config, and also adding a comment to remove this as soon as the component build is not relevant anymore.
lgtm
The CQ bit was checked by qsr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639713002/70001
Message was sent while issue was closed.
Committed patchset #3 (id:70001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e3b64d0d60ce004ba7976cbe5b55548475a81ea2 Cr-Commit-Position: refs/heads/master@{#299124} |
