Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2098073002:
Add layout test expectations for --isolate-extensions. (Closed)
|
Created:
4 years, 6 months ago by alexmos Modified:
4 years, 5 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, blink-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd layout test expectations for --isolate-extensions.
BUG=477150, 532666
Committed: https://crrev.com/be74185757ae2f3aef8afd46368811e94741c05b
Cr-Commit-Position: refs/heads/master@{#402305}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Charlie's comments #Messages
Total messages: 10 (3 generated)
creis@chromium.org changed reviewers: + creis@chromium.org
Thanks! Those were indeed the tests I failed on https://build.chromium.org/p/chromium.webkit/buildstatus?builder=WebKit%20Mac.... https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions (right): https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:1: # These tests currently fail when they run with --isolate-extensions. Is that accurate, or do you need to run them with --isolate-sites-for-testing=*.is? https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:8: http/tests/security/mixedContent/insecure-iframe-in-main-frame.html [ Failure ] Looks like https://crbug.com/582522 also applies to this one (per comment 2 on the bug). https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:16: fast/loader/form-state-restore-with-frames.html [ Failure ] Do you plan for the Win FYI bot to run the full suite of layout tests? We might want to revisit depending on how long it takes. :)
https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions (right): https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:1: # These tests currently fail when they run with --isolate-extensions. On 2016/06/24 23:26:02, Charlie Reis wrote: > Is that accurate, or do you need to run them with > --isolate-sites-for-testing=*.is? Indeed, they should run with --isolate-sites-for-testing=*.is. Do you think it's worth renaming the file itself to isolate-sites-for-testing? https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:8: http/tests/security/mixedContent/insecure-iframe-in-main-frame.html [ Failure ] On 2016/06/24 23:26:02, Charlie Reis wrote: > Looks like https://crbug.com/582522 also applies to this one (per comment 2 on > the bug). Yep, reference added. https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:16: fast/loader/form-state-restore-with-frames.html [ Failure ] On 2016/06/24 23:26:02, Charlie Reis wrote: > Do you plan for the Win FYI bot to run the full suite of layout tests? We might > want to revisit depending on how long it takes. :) I was going to, yes, but you're right that speed might be an issue, especially given how slow this bot already is. We can try the full set and see how it goes. Looks like PlzNavigate runs the full set on their bot, so I'm cautiously optimistic.
Thanks-- LGTM! https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions (right): https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:1: # These tests currently fail when they run with --isolate-extensions. On 2016/06/24 23:54:06, alexmos wrote: > On 2016/06/24 23:26:02, Charlie Reis wrote: > > Is that accurate, or do you need to run them with > > --isolate-sites-for-testing=*.is? > > Indeed, they should run with --isolate-sites-for-testing=*.is. > > Do you think it's worth renaming the file itself to isolate-sites-for-testing? Nah, we do the same thing with isolate-extensions.content_browsertests.filter.
On 2016/06/25 00:05:15, Charlie Reis wrote: > Thanks-- LGTM! > > https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions (right): > > https://codereview.chromium.org/2098073002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:1: # These > tests currently fail when they run with --isolate-extensions. > On 2016/06/24 23:54:06, alexmos wrote: > > On 2016/06/24 23:26:02, Charlie Reis wrote: > > > Is that accurate, or do you need to run them with > > > --isolate-sites-for-testing=*.is? > > > > Indeed, they should run with --isolate-sites-for-testing=*.is. > > > > Do you think it's worth renaming the file itself to isolate-sites-for-testing? > > Nah, we do the same thing with isolate-extensions.content_browsertests.filter. Great. To double-checked this list, I've just run the full set of layout tests with --isolate-sites-for-testing=*.is locally, and didn't see any new failures. There were a few failures that look like they are due to environment differences (e.g., fonts) rather than enabling OOPIFs. So I'm planning to land this and the followup CL (https://codereview.chromium.org/2100543002/) now and see what happens.
The CQ bit was checked by alexmos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add layout test expectations for --isolate-extensions. BUG=477150, 532666 ========== to ========== Add layout test expectations for --isolate-extensions. BUG=477150, 532666 Committed: https://crrev.com/be74185757ae2f3aef8afd46368811e94741c05b Cr-Commit-Position: refs/heads/master@{#402305} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/be74185757ae2f3aef8afd46368811e94741c05b Cr-Commit-Position: refs/heads/master@{#402305} |
