Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1147533005:
Disable DomSerializerTests.SerializeHTMLDOMWithDocType (Closed)
|
Created:
5 years, 7 months ago by Elly Fong-Jones Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable DomSerializerTests.SerializeHTMLDOMWithDocType
BUG=488495
R=brucedawson
TBR=jam
Committed: https://crrev.com/15a15d2d02b5638474c692fbbecb7da66b58f238
Cr-Commit-Position: refs/heads/master@{#330429}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment for disable #Patch Set 3 : Mac-only #Patch Set 4 : Use MAYBE pattern #Messages
Total messages: 18 (6 generated)
ellyjones@chromium.org changed reviewers: + brucedawson@chromium.org, jam@chromium.org
https://codereview.chromium.org/1147533005/diff/1/content/renderer/dom_serial... File content/renderer/dom_serializer_browsertest.cc (right): https://codereview.chromium.org/1147533005/diff/1/content/renderer/dom_serial... content/renderer/dom_serializer_browsertest.cc:781: // document type. Should add a comment here that links to the bug. See https://codereview.chromium.org/1014543006/diff/1/chrome/browser/extensions/a... for an example. Could also disable the test on OSX only since it is only flaky there, but since the bug itself appears to be portable I think unconditional disabling is fine also.
this looks mac only from the callstack, and i don't see any entries on http://chromium-try-flakes.appspot.com/search?q=DomSerializerTests.SerializeH... so at least make this if defined(os_macosx)
On 2015/05/18 15:33:13, jam wrote: > this looks mac only from the callstack, and i don't see any entries on > http://chromium-try-flakes.appspot.com/search?q=DomSerializerTests.SerializeH... > so at least make this if defined(os_macosx) I think this is broken on all platforms, but I've made the disable OSX only until we have evidence of that.
tommi@chromium.org changed reviewers: + tommi@chromium.org
lgtm
The CQ bit was checked by ellyjones@chromium.org
https://codereview.chromium.org/1147533005/diff/1/content/renderer/dom_serial... File content/renderer/dom_serializer_browsertest.cc (right): https://codereview.chromium.org/1147533005/diff/1/content/renderer/dom_serial... content/renderer/dom_serializer_browsertest.cc:781: // document type. On 2015/05/15 22:01:34, brucedawson wrote: > Should add a comment here that links to the bug. See > https://codereview.chromium.org/1014543006/diff/1/chrome/browser/extensions/a... > for an example. > > Could also disable the test on OSX only since it is only flaky there, but since > the bug itself appears to be portable I think unconditional disabling is fine > also. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1147533005/40001
The usual pattern for conditional disabling of tests seems to be a #define of MAYBE_TestName that is either TestName or DISABLE_TestName. But, I don't know if that is a 100% pattern or just partly. Anyway, lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Ah. I think that explains why the MAYBE_TestName pattern is used. The #if directives are inside of a macro expansion, which won't work. Either they need to be expanded and another line of code replicated, or else the MAYBE_TestName pattern is needed.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/1147533005/#ps60001 (title: "Use MAYBE pattern")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1147533005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/15a15d2d02b5638474c692fbbecb7da66b58f238 Cr-Commit-Position: refs/heads/master@{#330429} |
