Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968003003:
Stop silently accepting unsupported flags in test binaries (Closed)
|
Created:
3 years, 5 months ago by oprypin_webrtc Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, hlundin-webrtc, ehmaldonado_webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionStop silently accepting unsupported flags in test binaries
Instead explicitly ignore only the flags we know should be ignored.
BUG=webrtc:7568
Review-Url: https://codereview.webrtc.org/2968003003
Cr-Commit-Position: refs/heads/master@{#19412}
Committed: https://chromium.googlesource.com/external/webrtc/+/a2782f6f5d350e423b8166d603f44e17e1d7540b
Patch Set 1 : Remove "AllowCommandLineReparsing" #Patch Set 2 : Ignore Chromium-specific flags #Patch Set 3 : Ignore flags passed by xcodebuild #
Total comments: 2
Patch Set 4 : Fix a related TODO caused by unexpected flags #
Total comments: 1
Patch Set 5 : Add TODO #
Messages
Total messages: 38 (30 generated)
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/9434)
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Stop silently accepting unsupported flags in test binaries BUG=webrtc:7568 ========== to ========== Stop silently accepting unsupported flags in test binaries Instead explicitly ignore only the flags we know should be ignored. BUG=webrtc:7568 ==========
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CLs for remote refs other than refs/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CLs for remote refs other than refs/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
oprypin@webrtc.org changed reviewers: + henrika@webrtc.org, kjellander@webrtc.org
kjellander@, please also consider things that I might be missing (do perf bots perhaps pass some flags that we should also ignore?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yay this is excellent cleanup. Please file another bug and reference it with a TODO so we know what to clean up when we've made the buildbot recipes invoke the test without the extra flags (nor using runtest.py). LGTM https://codereview.webrtc.org/2968003003/diff/120001/tools_webrtc/valgrind/we... File tools_webrtc/valgrind/webrtc_tests.py (right): https://codereview.webrtc.org/2968003003/diff/120001/tools_webrtc/valgrind/we... tools_webrtc/valgrind/webrtc_tests.py:134: _, args = ignore_parser.parse_known_args(args) Just to check - these are needed since the memcheck bot isn't using tools_webrtc/gtest-parallel-wrapper.py, right? I'm leaning towards that we should work on getting rid of the flags being specified in the first place (due to the bot using runtest.py and the Chromium recipe modules), but we can punt that for the future for now if you add a reference to a cleanup bug here.
https://codereview.webrtc.org/2968003003/diff/120001/tools_webrtc/valgrind/we... File tools_webrtc/valgrind/webrtc_tests.py (right): https://codereview.webrtc.org/2968003003/diff/120001/tools_webrtc/valgrind/we... tools_webrtc/valgrind/webrtc_tests.py:134: _, args = ignore_parser.parse_known_args(args) On 2017/08/18 12:49:52, kjellander_webrtc wrote: > Just to check - these are needed since the memcheck bot isn't using > tools_webrtc/gtest-parallel-wrapper.py, right? I think so -- see patch set 1 where memcheck failed -- but it was a month ago :p > I'm leaning towards that we should work on getting rid of the flags being > specified in the first place (due to the bot using runtest.py and the Chromium > recipe modules), but we can punt that for the future for now if you add a > reference to a cleanup bug here. OK added reference https://codereview.webrtc.org/2968003003/diff/40002/webrtc/test/test_main.cc File webrtc/test/test_main.cc (right): https://codereview.webrtc.org/2968003003/diff/40002/webrtc/test/test_main.cc#... webrtc/test/test_main.cc:26: "Intentionally ignored flag intended for iOS simulator."); Note that this is passed on a lower level - from xcodebuild. Test infra is not involved, this is reproducible locally.
Don't know the details but lgtm
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2968003003/#ps150001 (title: "Add TODO")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 150001, "attempt_start_ts": 1503063862122810,
"parent_rev": "81bc523f5d7eab43796373954b2aaf17fa65ad35", "commit_rev":
"a2782f6f5d350e423b8166d603f44e17e1d7540b"}
Message was sent while issue was closed.
Description was changed from ========== Stop silently accepting unsupported flags in test binaries Instead explicitly ignore only the flags we know should be ignored. BUG=webrtc:7568 ========== to ========== Stop silently accepting unsupported flags in test binaries Instead explicitly ignore only the flags we know should be ignored. BUG=webrtc:7568 Review-Url: https://codereview.webrtc.org/2968003003 Cr-Commit-Position: refs/heads/master@{#19412} Committed: https://chromium.googlesource.com/external/webrtc/+/a2782f6f5d350e423b8166d60... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:150001) as https://chromium.googlesource.com/external/webrtc/+/a2782f6f5d350e423b8166d60...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:150001) has been created in https://codereview.webrtc.org/3002963002/ by oprypin@webrtc.org. The reason for reverting is: Causes failures on perf bots https://luci-milo.appspot.com/buildbot/client.webrtc.perf/Mac%2010.11/3567. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
