Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
744973002:
Pass command line args to apptests (Closed)
|
Created:
6 years, 1 month ago by Chris Masone Modified:
6 years ago CC:
mojo-reviews_chromium.org, 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 Project:
mojo Visibility:
Public. |
DescriptionPass command line args to apptests
Apptests should be able to access passed on the command line when they're run.
The test runner will now export the args and cause them to be handed to
the ApplicationImpl created as a part of the ApplicationTestBase class.
BUG=None
TEST=run app tests with apptest_runner.py
R=msw@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/c15c806c42e5e97b90bb4816c7601c8ab58fa8ad
Patch Set 1 #
Total comments: 11
Patch Set 2 : switch to using --args-for to pass arguments to apptests #
Total comments: 15
Patch Set 3 : rebase and address comments #
Total comments: 1
Messages
Total messages: 16 (2 generated)
cmasone@chromium.org changed reviewers: + msw@chromium.org
eh?
https://codereview.chromium.org/744973002/diff/1/examples/apptest/example_app... File examples/apptest/example_apptest.cc (right): https://codereview.chromium.org/744973002/diff/1/examples/apptest/example_app... examples/apptest/example_apptest.cc:20: ExampleApplicationTest() : ApplicationTestBase(mojo::test::Args().Clone()) {} Maybe ApplicationTestBase should use the real arguments by default and have a SetUpWithArgs() helper for subclass customization? https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... File mojo/public/cpp/application/lib/application_test_base.cc (right): https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... mojo/public/cpp/application/lib/application_test_base.cc:20: // Command-line args passed to the harness are available to any test The args are actually passed to the test application like this, right? $ mojo_shell "mojo:test_app passed_arg" not_passed_shell_arg If so, consider revising the comment, perhaps something like "// Share the application command-line arguments with multiple application tests." https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... mojo/public/cpp/application/lib/application_test_base.cc:39: const Array<String>& Args() { nit: can this be inlined in the header as args()? https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... mojo/public/cpp/application/lib/application_test_base.cc:46: if (arg) When are these ever null? https://codereview.chromium.org/744973002/diff/1/mojo/tools/apptest_runner.py File mojo/tools/apptest_runner.py (right): https://codereview.chromium.org/744973002/diff/1/mojo/tools/apptest_runner.py... mojo/tools/apptest_runner.py:67: command_line = [mojo_shell, '--example_apptest_arg', 1) Does this actually work? I thought the only args passed to the application must be parts of the single application argument to the shell, like --gtest_filter=* below (or using the --args-for swtich). 2) Exemplify/test --args-for here as well. 3) nit: add a comment here and a TODO for proper testing.
https://codereview.chromium.org/744973002/diff/1/examples/apptest/example_app... File examples/apptest/example_apptest.cc (right): https://codereview.chromium.org/744973002/diff/1/examples/apptest/example_app... examples/apptest/example_apptest.cc:20: ExampleApplicationTest() : ApplicationTestBase(mojo::test::Args().Clone()) {} On 2014/11/20 21:59:12, msw wrote: > Maybe ApplicationTestBase should use the real arguments by default and have a > SetUpWithArgs() helper for subclass customization? Done. https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... File mojo/public/cpp/application/lib/application_test_base.cc (right): https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... mojo/public/cpp/application/lib/application_test_base.cc:39: const Array<String>& Args() { On 2014/11/20 21:59:12, msw wrote: > nit: can this be inlined in the header as args()? I actually don't think so, as g_args is defined only here in the .cc file. https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... mojo/public/cpp/application/lib/application_test_base.cc:46: if (arg) On 2014/11/20 21:59:12, msw wrote: > When are these ever null? After processing by the gtest library, the last arg is. I could use a loop index instead, if you'd rather, to only push_back() the first argc items. https://codereview.chromium.org/744973002/diff/1/mojo/tools/apptest_runner.py File mojo/tools/apptest_runner.py (right): https://codereview.chromium.org/744973002/diff/1/mojo/tools/apptest_runner.py... mojo/tools/apptest_runner.py:67: command_line = [mojo_shell, '--example_apptest_arg', On 2014/11/20 21:59:12, msw wrote: > 1) Does this actually work? I thought the only args passed to the application > must be parts of the single application argument to the shell, like > --gtest_filter=* below (or using the --args-for swtich). You were right, this wasn't actually working. > > 2) Exemplify/test --args-for here as well. > After discussion in chat, using only --args-for right now. > 3) nit: add a comment here and a TODO for proper testing. Done
Sorry for the response delay! https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... File mojo/public/cpp/application/lib/application_test_base.cc (right): https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... mojo/public/cpp/application/lib/application_test_base.cc:39: const Array<String>& Args() { On 2014/11/20 23:35:19, Chris Masone wrote: > On 2014/11/20 21:59:12, msw wrote: > > nit: can this be inlined in the header as args()? > > I actually don't think so, as g_args is defined only here in the .cc file. Acknowledged. https://codereview.chromium.org/744973002/diff/1/mojo/public/cpp/application/... mojo/public/cpp/application/lib/application_test_base.cc:46: if (arg) On 2014/11/20 23:35:19, Chris Masone wrote: > On 2014/11/20 21:59:12, msw wrote: > > When are these ever null? > > After processing by the gtest library, the last arg is. I could use a loop index > instead, if you'd rather, to only push_back() the first argc items. Ah, OK. This is fine as-is. https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... File mojo/public/cpp/application/application_test_base.h (right): https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/application_test_base.h:24: const Array<String>& Args(); nit: Add a comment like "Access the command line arguments passed to the application test." https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/application_test_base.h:39: void SetUpWithArgs(Array<String> args); nit: Add a comment, like "A testing::Test::SetUp helper to override the application command line arguments." https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... File mojo/public/cpp/application/lib/application_test_base.cc (right): https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/lib/application_test_base.cc:50: : args_(mojo::test::Args().Clone()), application_impl_(nullptr) { Namespace not needed. https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/lib/application_test_base.cc:58: SetUp(); Flip this around, have SetUp call SetUpWithArgs(Args().Clone) and remove the |args_| member. https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... File mojo/public/cpp/application/lib/application_test_main.cc (right): https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/lib/application_test_main.cc:32: // TODO(msw): Provide tests access to these actual command line arguments. Remove this TODO now. https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... File mojo/tools/apptest_runner.py (right): https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... mojo/tools/apptest_runner.py:70: "--args-for={0} --example_apptest_arg --gtest_filter={1}".format( Is this quoting correct? I think it should be --args-for="..."
https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... File mojo/public/cpp/application/application_test_base.h (right): https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/application_test_base.h:24: const Array<String>& Args(); On 2014/11/22 18:23:38, msw wrote: > nit: Add a comment like "Access the command line arguments passed to the > application test." Done. https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/application_test_base.h:39: void SetUpWithArgs(Array<String> args); On 2014/11/22 18:23:38, msw wrote: > nit: Add a comment, like "A testing::Test::SetUp helper to override the > application command line arguments." Done. https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... File mojo/public/cpp/application/lib/application_test_base.cc (right): https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/lib/application_test_base.cc:50: : args_(mojo::test::Args().Clone()), application_impl_(nullptr) { On 2014/11/22 18:23:38, msw wrote: > Namespace not needed. Done. https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/lib/application_test_base.cc:58: SetUp(); On 2014/11/22 18:23:38, msw wrote: > Flip this around, have SetUp call SetUpWithArgs(Args().Clone) and remove the > |args_| member. Done. https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... File mojo/public/cpp/application/lib/application_test_main.cc (right): https://codereview.chromium.org/744973002/diff/20001/mojo/public/cpp/applicat... mojo/public/cpp/application/lib/application_test_main.cc:32: // TODO(msw): Provide tests access to these actual command line arguments. On 2014/11/22 18:23:39, msw wrote: > Remove this TODO now. Done. https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... File mojo/tools/apptest_runner.py (right): https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... mojo/tools/apptest_runner.py:70: "--args-for={0} --example_apptest_arg --gtest_filter={1}".format( On 2014/11/22 18:23:39, msw wrote: > Is this quoting correct? I think it should be --args-for="..." Since the command isn't being interpreted by a shell when we run it below, I believe this is the right quoting.
lgtm with quoting fix https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... File mojo/tools/apptest_runner.py (right): https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... mojo/tools/apptest_runner.py:70: "--args-for={0} --example_apptest_arg --gtest_filter={1}".format( On 2014/11/22 20:14:44, Chris Masone wrote: > On 2014/11/22 18:23:39, msw wrote: > > Is this quoting correct? I think it should be --args-for="..." > > Since the command isn't being interpreted by a shell when we run it below, I > believe this is the right quoting. I think you still want escaped quotes around the value of the args-for switch.
https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... File mojo/tools/apptest_runner.py (right): https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... mojo/tools/apptest_runner.py:70: "--args-for={0} --example_apptest_arg --gtest_filter={1}".format( On 2014/11/22 21:46:54, msw wrote: > On 2014/11/22 20:14:44, Chris Masone wrote: > > On 2014/11/22 18:23:39, msw wrote: > > > Is this quoting correct? I think it should be --args-for="..." > > > > Since the command isn't being interpreted by a shell when we run it below, I > > believe this is the right quoting. > > I think you still want escaped quotes around the value of the args-for switch. No, then you wind up with the quotes embedded in the contents of --args-for, and mojo_shell complains thusly: [ERROR:mojo_main.cc(47)] Error: invalid URL: "mojo:example_apptests and you wind up with no args because of this error.
On 2014/11/24 17:08:22, Chris Masone wrote: > https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... > File mojo/tools/apptest_runner.py (right): > > https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... > mojo/tools/apptest_runner.py:70: "--args-for={0} --example_apptest_arg > --gtest_filter={1}".format( > On 2014/11/22 21:46:54, msw wrote: > > On 2014/11/22 20:14:44, Chris Masone wrote: > > > On 2014/11/22 18:23:39, msw wrote: > > > > Is this quoting correct? I think it should be --args-for="..." > > > > > > Since the command isn't being interpreted by a shell when we run it below, I > > > believe this is the right quoting. > > > > I think you still want escaped quotes around the value of the args-for switch. > > No, then you wind up with the quotes embedded in the contents of --args-for, and > mojo_shell complains thusly: > > [ERROR:mojo_main.cc(47)] Error: invalid URL: "mojo:example_apptests > > and you wind up with no args because of this error. I should have asked, given the above, does this LGTY as-is?
lgtm https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... File mojo/tools/apptest_runner.py (right): https://codereview.chromium.org/744973002/diff/20001/mojo/tools/apptest_runne... mojo/tools/apptest_runner.py:70: "--args-for={0} --example_apptest_arg --gtest_filter={1}".format( On 2014/11/24 17:08:22, Chris Masone wrote: > On 2014/11/22 21:46:54, msw wrote: > > On 2014/11/22 20:14:44, Chris Masone wrote: > > > On 2014/11/22 18:23:39, msw wrote: > > > > Is this quoting correct? I think it should be --args-for="..." > > > > > > Since the command isn't being interpreted by a shell when we run it below, I > > > believe this is the right quoting. > > > > I think you still want escaped quotes around the value of the args-for switch. > > No, then you wind up with the quotes embedded in the contents of --args-for, and > mojo_shell complains thusly: > > [ERROR:mojo_main.cc(47)] Error: invalid URL: "mojo:example_apptests > > and you wind up with no args because of this error. Odd, both quoting styles work with direct command line usage, but I confirmed your findings wrt this invocation.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as c15c806c42e5e97b90bb4816c7601c8ab58fa8ad (presubmit successful).
Message was sent while issue was closed.
jamesr@chromium.org changed reviewers: + jamesr@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/744973002/diff/40001/mojo/public/cpp/applicat... File mojo/public/cpp/application/lib/application_test_main.cc (right): https://codereview.chromium.org/744973002/diff/40001/mojo/public/cpp/applicat... mojo/public/cpp/application/lib/application_test_main.cc:24: // $ mojo_shell mojo:example_apptests \ this breaks the android build: FAILED: /b/build/goma/gomacc ../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -MMD -MF obj/mojo/public/cpp/application/lib/test_support_standalone.application_test_main.o.d -DCHROMIUM_BUILD -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=1 -DENABLE_NOTIFICATIONS -DENABLE_BROWSER_CDMS -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC -DDISABLE_NACL -DENABLE_CONFIGURATION_POLICY -DENABLE_MANAGED_USERS=1 -DENABLE_AUTOFILL_DIALOG=1 -D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DUSE_STLPORT=1 -D_STLP_USE_PTR_SPECIALIZATIONS=1 -D__GNU_SOURCE=1 -DNDEBUG -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0 -DGTEST_HAS_RTTI=0 -DGTEST_USE_OWN_TR1_TUPLE=1 -DGTEST_HAS_TR1_TUPLE=1 -I../.. -Igen -I../../testing/gtest/include -fno-strict-aliasing -march=armv7-a -mfloat-abi=softfp -mtune=generic-armv7-a -mthumb -mthumb-interwork -fno-tree-sra -fno-caller-saves -funwind-tables -fPIC -pipe -ffunction-sections -funwind-tables -fno-short-enums -finline-limit=64 -mfpu=vfpv3-d16 -Wall -Wsign-compare -Wendif-labels -Werror -Wno-missing-field-initializers -Wno-unused-parameter -Wno-psabi -Wno-extra -Wno-ignored-qualifiers -Wno-type-limits -Wno-unused-local-typedefs -isystem../../third_party/android_tools/ndk/sources/cxx-stl/stlport/stlport -fvisibility=hidden --sysroot=/b/build/slave/mojo/build/src/third_party/android_tools/ndk/platforms/android-14/arch-arm -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -Os -g2 -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -Wno-narrowing -Wno-literal-suffix -Wno-error=c++0x-compat -Wno-non-virtual-dtor -Wno-sign-promo -fno-rtti -fno-exceptions -c ../../mojo/public/cpp/application/lib/application_test_main.cc -o obj/mojo/public/cpp/application/lib/test_support_standalone.application_test_main.o ../../mojo/public/cpp/application/lib/application_test_main.cc:24:5: error: multi-line comment [-Werror=comment] // $ mojo_shell mojo:example_apptests \ ^ cc1plus: all warnings being treated as errors (yes we should have trybots - will soon)
Message was sent while issue was closed.
On 2014/11/24 20:22:40, jamesr wrote: > https://codereview.chromium.org/744973002/diff/40001/mojo/public/cpp/applicat... > File mojo/public/cpp/application/lib/application_test_main.cc (right): > > https://codereview.chromium.org/744973002/diff/40001/mojo/public/cpp/applicat... > mojo/public/cpp/application/lib/application_test_main.cc:24: // $ mojo_shell > mojo:example_apptests \ > this breaks the android build: > > FAILED: /b/build/goma/gomacc > ../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ > -MMD -MF > obj/mojo/public/cpp/application/lib/test_support_standalone.application_test_main.o.d > -DCHROMIUM_BUILD -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=1 > -DENABLE_NOTIFICATIONS -DENABLE_BROWSER_CDMS -DENABLE_PRINTING=1 > -DENABLE_BASIC_PRINTING=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC > -DDISABLE_NACL -DENABLE_CONFIGURATION_POLICY -DENABLE_MANAGED_USERS=1 > -DENABLE_AUTOFILL_DIALOG=1 -D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H > -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DUSE_STLPORT=1 > -D_STLP_USE_PTR_SPECIALIZATIONS=1 -D__GNU_SOURCE=1 -DNDEBUG > -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0 -DGTEST_HAS_RTTI=0 > -DGTEST_USE_OWN_TR1_TUPLE=1 -DGTEST_HAS_TR1_TUPLE=1 -I../.. -Igen > -I../../testing/gtest/include -fno-strict-aliasing -march=armv7-a > -mfloat-abi=softfp -mtune=generic-armv7-a -mthumb -mthumb-interwork > -fno-tree-sra -fno-caller-saves -funwind-tables -fPIC -pipe -ffunction-sections > -funwind-tables -fno-short-enums -finline-limit=64 -mfpu=vfpv3-d16 -Wall > -Wsign-compare -Wendif-labels -Werror -Wno-missing-field-initializers > -Wno-unused-parameter -Wno-psabi -Wno-extra -Wno-ignored-qualifiers > -Wno-type-limits -Wno-unused-local-typedefs > -isystem../../third_party/android_tools/ndk/sources/cxx-stl/stlport/stlport > -fvisibility=hidden > --sysroot=/b/build/slave/mojo/build/src/third_party/android_tools/ndk/platforms/android-14/arch-arm > -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -Os -g2 > -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -Wno-narrowing > -Wno-literal-suffix -Wno-error=c++0x-compat -Wno-non-virtual-dtor > -Wno-sign-promo -fno-rtti -fno-exceptions -c > ../../mojo/public/cpp/application/lib/application_test_main.cc -o > obj/mojo/public/cpp/application/lib/test_support_standalone.application_test_main.o > ../../mojo/public/cpp/application/lib/application_test_main.cc:24:5: error: > multi-line comment [-Werror=comment] > // $ mojo_shell mojo:example_apptests \ > ^ > cc1plus: all warnings being treated as errors > > (yes we should have trybots - will soon) The \ at the end (which is actually supposed to a part of the recommended command line) is what triggers this, yes? The Right Fix is to do /*...*/, correct?
Message was sent while issue was closed.
On 2014/11/24 20:29:13, Chris Masone wrote: > On 2014/11/24 20:22:40, jamesr wrote: > > > https://codereview.chromium.org/744973002/diff/40001/mojo/public/cpp/applicat... > > File mojo/public/cpp/application/lib/application_test_main.cc (right): > > > > > https://codereview.chromium.org/744973002/diff/40001/mojo/public/cpp/applicat... > > mojo/public/cpp/application/lib/application_test_main.cc:24: // $ mojo_shell > > mojo:example_apptests \ > > this breaks the android build: > > > > FAILED: /b/build/goma/gomacc > > > ../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ > > -MMD -MF > > > obj/mojo/public/cpp/application/lib/test_support_standalone.application_test_main.o.d > > -DCHROMIUM_BUILD -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=1 > > -DENABLE_NOTIFICATIONS -DENABLE_BROWSER_CDMS -DENABLE_PRINTING=1 > > -DENABLE_BASIC_PRINTING=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC > > -DDISABLE_NACL -DENABLE_CONFIGURATION_POLICY -DENABLE_MANAGED_USERS=1 > > -DENABLE_AUTOFILL_DIALOG=1 -D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H > > -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DUSE_STLPORT=1 > > -D_STLP_USE_PTR_SPECIALIZATIONS=1 -D__GNU_SOURCE=1 -DNDEBUG > > -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0 -DGTEST_HAS_RTTI=0 > > -DGTEST_USE_OWN_TR1_TUPLE=1 -DGTEST_HAS_TR1_TUPLE=1 -I../.. -Igen > > -I../../testing/gtest/include -fno-strict-aliasing -march=armv7-a > > -mfloat-abi=softfp -mtune=generic-armv7-a -mthumb -mthumb-interwork > > -fno-tree-sra -fno-caller-saves -funwind-tables -fPIC -pipe > -ffunction-sections > > -funwind-tables -fno-short-enums -finline-limit=64 -mfpu=vfpv3-d16 -Wall > > -Wsign-compare -Wendif-labels -Werror -Wno-missing-field-initializers > > -Wno-unused-parameter -Wno-psabi -Wno-extra -Wno-ignored-qualifiers > > -Wno-type-limits -Wno-unused-local-typedefs > > -isystem../../third_party/android_tools/ndk/sources/cxx-stl/stlport/stlport > > -fvisibility=hidden > > > --sysroot=/b/build/slave/mojo/build/src/third_party/android_tools/ndk/platforms/android-14/arch-arm > > -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -Os -g2 > > -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 > -Wno-narrowing > > -Wno-literal-suffix -Wno-error=c++0x-compat -Wno-non-virtual-dtor > > -Wno-sign-promo -fno-rtti -fno-exceptions -c > > ../../mojo/public/cpp/application/lib/application_test_main.cc -o > > > obj/mojo/public/cpp/application/lib/test_support_standalone.application_test_main.o > > ../../mojo/public/cpp/application/lib/application_test_main.cc:24:5: error: > > multi-line comment [-Werror=comment] > > // $ mojo_shell mojo:example_apptests \ > > ^ > > cc1plus: all warnings being treated as errors > > > > (yes we should have trybots - will soon) > > The \ at the end (which is actually supposed to a part of the recommended > command line) is what triggers this, yes? > > The Right Fix is to do /*...*/, correct? Actually...I guess not, since that's not consistent with the rest of the file. Suggestions for properly writing this comment?
Message was sent while issue was closed.
I'm not sure how to write this comment, sorry :( |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
