Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1808643005:
Truly disable tests. (Closed)
|
Created:
4 years, 9 months ago by stefan-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTruly disable tests.
...which weren't successfully disabled in https://chromium.googlesource.com/external/webrtc.git/+/55d6e7ca5f685af32737c460687be39f8eb7b9e2
TBR=kjellander@webrtc.org, torbjorng@webrtc.org
BUG=webrtc:5659
Committed: https://crrev.com/102362b79002ca93b5ab30e6f235e4bec4b2abf4
Cr-Commit-Position: refs/heads/master@{#12049}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Comments addressed. #
Messages
Total messages: 17 (5 generated)
stefan@webrtc.org changed reviewers: + kjellander@webrtc.org, torbjorng@webrtc.org
The previous CL apparently didn't do much. Now they should be disabled for real.
Description was changed from ========== Truly disable tests. BUG=webrtc:5659 ========== to ========== Truly disable tests. ...which weren't successfully disabled in https://chromium.googlesource.com/external/webrtc.git/+/55d6e7ca5f685af32737c... BUG=webrtc:5659 ==========
https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1782: #if defined(WIN) && !defined(NDEBUG) Why inverse the NDEBUG check instead of _DEBUG? Both are clearly set in https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... but I think double-negations are harder to read. I'm not even 100% it's set on Windows, by looking at the GYP code in there. https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1782: #if defined(WIN) && !defined(NDEBUG) Use WEBRTC_WIN instead of WIN. Applies to whole CL.
https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1782: #if defined(WIN) && !defined(NDEBUG) On 2016/03/17 20:52:34, kjellander (webrtc) wrote: > Why inverse the NDEBUG check instead of _DEBUG? > > Both are clearly set in > https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... > but I think double-negations are harder to read. I'm not even 100% it's set on > Windows, by looking at the GYP code in there. The previous CL didn't work, as The previous CL didn't work (see https://build.chromium.org/p/client.webrtc/builders/Win64%20Debug). Not sure why, and I didn't have time to investigate in detail. I however tried using !NDEBUG on linux and that did work, so I assumed that was the problem. Maybe the true problem was WIN vs. WEBRTC_WIN? Btw, any clue why the try bot doesn't trigger the issue we're trying to hide? I thought it would, that's why I submitted the previous CL.
On 2016/03/17 21:07:15, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... > File webrtc/api/peerconnection_unittest.cc (right): > > https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... > webrtc/api/peerconnection_unittest.cc:1782: #if defined(WIN) && !defined(NDEBUG) > On 2016/03/17 20:52:34, kjellander (webrtc) wrote: > > Why inverse the NDEBUG check instead of _DEBUG? > > > > Both are clearly set in > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... > > but I think double-negations are harder to read. I'm not even 100% it's set on > > Windows, by looking at the GYP code in there. > > > The previous CL didn't work, as The previous CL didn't work (see > https://build.chromium.org/p/client.webrtc/builders/Win64%20Debug). Not sure > why, and I didn't have time to investigate in detail. I however tried using > !NDEBUG on linux and that did work, so I assumed that was the problem. Maybe the > true problem was WIN vs. WEBRTC_WIN? > > Btw, any clue why the try bot doesn't trigger the issue we're trying to hide? I > thought it would, that's why I submitted the previous CL. I don't know either. I tried triggering the issue through the Chromium trybots (since Chromium's thread local storage works the same way) but no dice. So how is the trybot's configuration different than the failing bot's (and my local configuration)? It would be good to know for future reference.
On 2016/03/18 00:42:26, Taylor Brandstetter wrote: > On 2016/03/17 21:07:15, stefan-webrtc (holmer) wrote: > > > https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... > > File webrtc/api/peerconnection_unittest.cc (right): > > > > > https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... > > webrtc/api/peerconnection_unittest.cc:1782: #if defined(WIN) && > !defined(NDEBUG) > > On 2016/03/17 20:52:34, kjellander (webrtc) wrote: > > > Why inverse the NDEBUG check instead of _DEBUG? > > > > > > Both are clearly set in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... > > > but I think double-negations are harder to read. I'm not even 100% it's set > on > > > Windows, by looking at the GYP code in there. > > > > > > The previous CL didn't work, as The previous CL didn't work (see > > https://build.chromium.org/p/client.webrtc/builders/Win64%20Debug). Not sure > > why, and I didn't have time to investigate in detail. I however tried using > > !NDEBUG on linux and that did work, so I assumed that was the problem. Maybe > the > > true problem was WIN vs. WEBRTC_WIN? > > > > Btw, any clue why the try bot doesn't trigger the issue we're trying to hide? > I > > thought it would, that's why I submitted the previous CL. > > I don't know either. I tried triggering the issue through the Chromium trybots > (since Chromium's thread local storage works the same way) but no dice. So how > is the trybot's configuration different than the failing bot's (and my local > configuration)? It would be good to know for future reference. I have no idea. I even compared the GYP_DFEINES for the runhooks steps between the bots and they're identical. The only thing I can think of is cached build artifacts on one of the hosts (but the landmines functionality should have detected the landmine and clobbered the build at the MSVS switch CL already). You could try triggering a new run with clobbering using: git cl try --bot=win_x64_dbg --clobber -m tryserver.webrtc.
https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1782: #if defined(WIN) && !defined(NDEBUG) On 2016/03/17 21:07:15, stefan-webrtc (holmer) wrote: > On 2016/03/17 20:52:34, kjellander (webrtc) wrote: > > Why inverse the NDEBUG check instead of _DEBUG? > > > > Both are clearly set in > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... > > but I think double-negations are harder to read. I'm not even 100% it's set on > > Windows, by looking at the GYP code in there. > > > The previous CL didn't work, as The previous CL didn't work (see > https://build.chromium.org/p/client.webrtc/builders/Win64%20Debug). Not sure > why, and I didn't have time to investigate in detail. I however tried using > !NDEBUG on linux and that did work, so I assumed that was the problem. Maybe the > true problem was WIN vs. WEBRTC_WIN? I'm sorry you had to waste so much time on this. I also can't explain the trybot behavior (see previous reply). I don't know in detail which defines are set so I cannot guarantee anything without local testing. I wouldn't dare to trust that all defines are equal for Linux and Windows though, since the build/common.gypi is quite hard to read. > > Btw, any clue why the try bot doesn't trigger the issue we're trying to hide? I > thought it would, that's why I submitted the previous CL. See previous reply.
Comments addressed.
https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1808643005/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1782: #if defined(WIN) && !defined(NDEBUG) On 2016/03/18 04:49:10, kjellander (webrtc) wrote: > On 2016/03/17 21:07:15, stefan-webrtc (holmer) wrote: > > On 2016/03/17 20:52:34, kjellander (webrtc) wrote: > > > Why inverse the NDEBUG check instead of _DEBUG? > > > > > > Both are clearly set in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... > > > but I think double-negations are harder to read. I'm not even 100% it's set > on > > > Windows, by looking at the GYP code in there. > > > > > > The previous CL didn't work, as The previous CL didn't work (see > > https://build.chromium.org/p/client.webrtc/builders/Win64%20Debug). Not sure > > why, and I didn't have time to investigate in detail. I however tried using > > !NDEBUG on linux and that did work, so I assumed that was the problem. Maybe > the > > true problem was WIN vs. WEBRTC_WIN? > > I'm sorry you had to waste so much time on this. I also can't explain the trybot > behavior (see previous reply). > I don't know in detail which defines are set so I cannot guarantee anything > without local testing. > I wouldn't dare to trust that all defines are equal for Linux and Windows > though, since the build/common.gypi is quite hard to read. > > > > > Btw, any clue why the try bot doesn't trigger the issue we're trying to hide? > I > > thought it would, that's why I submitted the previous CL. > > See previous reply. Going for WEBRTC_WIN and _DEBUG.
PTAL
Description was changed from ========== Truly disable tests. ...which weren't successfully disabled in https://chromium.googlesource.com/external/webrtc.git/+/55d6e7ca5f685af32737c... BUG=webrtc:5659 ========== to ========== Truly disable tests. ...which weren't successfully disabled in https://chromium.googlesource.com/external/webrtc.git/+/55d6e7ca5f685af32737c... TBR=kjellander@webrtc.org, torbjorng@webrtc.org BUG=webrtc:5659 ==========
TBRing so that the bots gets green.
Description was changed from ========== Truly disable tests. ...which weren't successfully disabled in https://chromium.googlesource.com/external/webrtc.git/+/55d6e7ca5f685af32737c... TBR=kjellander@webrtc.org, torbjorng@webrtc.org BUG=webrtc:5659 ========== to ========== Truly disable tests. ...which weren't successfully disabled in https://chromium.googlesource.com/external/webrtc.git/+/55d6e7ca5f685af32737c... TBR=kjellander@webrtc.org, torbjorng@webrtc.org BUG=webrtc:5659 Committed: https://chromium.googlesource.com/external/webrtc/+/102362b79002ca93b5ab30e6f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 102362b79002ca93b5ab30e6f235e4bec4b2abf4 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Truly disable tests. ...which weren't successfully disabled in https://chromium.googlesource.com/external/webrtc.git/+/55d6e7ca5f685af32737c... TBR=kjellander@webrtc.org, torbjorng@webrtc.org BUG=webrtc:5659 Committed: https://chromium.googlesource.com/external/webrtc/+/102362b79002ca93b5ab30e6f... ========== to ========== Truly disable tests. ...which weren't successfully disabled in https://chromium.googlesource.com/external/webrtc.git/+/55d6e7ca5f685af32737c... TBR=kjellander@webrtc.org, torbjorng@webrtc.org BUG=webrtc:5659 Committed: https://crrev.com/102362b79002ca93b5ab30e6f235e4bec4b2abf4 Cr-Commit-Position: refs/heads/master@{#12049} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/102362b79002ca93b5ab30e6f235e4bec4b2abf4 Cr-Commit-Position: refs/heads/master@{#12049} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
