Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2911153002:
Allow use of ScopedAllowIO in browser tests (Closed)
DescriptionAllow use of ScopedAllowIO in browser tests
This change is based on the discussion on a chormium-dev
thread titled "ScopedAllowIO ban isn't preventing usage,
it's just annoying people" where jam@ mentioned "Excluding
test files seems like a good change to me."
TEST=tested locally by adding ScopedAllowIO to a random browser test file with and without this change
Review-Url: https://codereview.chromium.org/2911153002
Cr-Commit-Position: refs/heads/master@{#476211}
Committed: https://chromium.googlesource.com/chromium/src/+/e1396f8adefd2f71ed33732f5ffe013bcf87990a
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase #Messages
Total messages: 22 (13 generated)
satorux@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py#newcode200 PRESUBMIT.py:200: r"^.*browser(|_)test[a-z_]*\.cc$", This pattern is based on the existing pattern r"^content[\\\/].*browser(|_)test[a-zA-Z_]*\.cc$", but removed A-Z since it does not make a difference.
The CQ bit was checked by satorux@chromium.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.chromium.or...
Description was changed from ========== Allow use of ScopedAllowIO in browser tests This change is based on the discussion on a chormium-dev thread titled "ScopedAllowIO ban isn't preventing usage, it's just annoying people" ========== to ========== Allow use of ScopedAllowIO in browser tests This change is based on the discussion on a chormium-dev thread titled "ScopedAllowIO ban isn't preventing usage, it's just annoying people" where jam@ mentioned "Excluding test files seems like a good change to me." ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Allow use of ScopedAllowIO in browser tests This change is based on the discussion on a chormium-dev thread titled "ScopedAllowIO ban isn't preventing usage, it's just annoying people" where jam@ mentioned "Excluding test files seems like a good change to me." ========== to ========== Allow use of ScopedAllowIO in browser tests This change is based on the discussion on a chormium-dev thread titled "ScopedAllowIO ban isn't preventing usage, it's just annoying people" where jam@ mentioned "Excluding test files seems like a good change to me." TEST=tested locally by adding ScopedAllowIO to a random browser test file with and without this change ==========
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py#newcode200 PRESUBMIT.py:200: r"^.*browser(|_)test[a-z_]*\.cc$", On 2017/05/30 04:33:20, satorux1 wrote: > This pattern is based on the existing pattern > > r"^content[\\\/].*browser(|_)test[a-zA-Z_]*\.cc$", > > but removed A-Z since it does not make a difference. Could we extend this to all files with "test" in the name (drop "browser")? This affects unit tests too (my original case was TestingProfile which is used by unit tests).
On 2017/05/30 05:35:56, Matt Giuca wrote: > https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py > File PRESUBMIT.py (right): > > https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py#newcode200 > PRESUBMIT.py:200: r"^.*browser(|_)test[a-z_]*\.cc$", > On 2017/05/30 04:33:20, satorux1 wrote: > > This pattern is based on the existing pattern > > > > r"^content[\\\/].*browser(|_)test[a-zA-Z_]*\.cc$", > > > > but removed A-Z since it does not make a difference. > > Could we extend this to all files with "test" in the name (drop "browser")? This > affects unit tests too (my original case was TestingProfile which is used by > unit tests). I thought *unittest.cc didn't need ScopedAllowIO? https://cs.chromium.org/search/?q=ScopedAllowIO+file:unittest.cc$+package:%5E... => 0 https://cs.chromium.org/search/?q=ScopedAllowIO+file:browsertest.cc$+package:... => 70
https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py#newcode200 PRESUBMIT.py:200: r"^.*browser(|_)test[a-z_]*\.cc$", On 2017/05/30 05:35:56, Matt Giuca wrote: > On 2017/05/30 04:33:20, satorux1 wrote: > > This pattern is based on the existing pattern > > > > r"^content[\\\/].*browser(|_)test[a-zA-Z_]*\.cc$", > > > > but removed A-Z since it does not make a difference. > > Could we extend this to all files with "test" in the name (drop "browser")? This > affects unit tests too (my original case was TestingProfile which is used by > unit tests). I guess there's no point in allowing it currently if nobody is using it. I thought unit tests needed it because they have no threads; therefore if they run normally-multithreaded code on one thread they have to use ScopedAllowIO. They do indirectly use it through TestingProfile but maybe that's it.
On 2017/05/30 05:48:01, Matt Giuca wrote: > https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py > File PRESUBMIT.py (right): > > https://codereview.chromium.org/2911153002/diff/1/PRESUBMIT.py#newcode200 > PRESUBMIT.py:200: r"^.*browser(|_)test[a-z_]*\.cc$", > On 2017/05/30 05:35:56, Matt Giuca wrote: > > On 2017/05/30 04:33:20, satorux1 wrote: > > > This pattern is based on the existing pattern > > > > > > r"^content[\\\/].*browser(|_)test[a-zA-Z_]*\.cc$", > > > > > > but removed A-Z since it does not make a difference. > > > > Could we extend this to all files with "test" in the name (drop "browser")? > This > > affects unit tests too (my original case was TestingProfile which is used by > > unit tests). > > I guess there's no point in allowing it currently if nobody is using it. > > I thought unit tests needed it because they have no threads; therefore if they > run normally-multithreaded code on one thread they have to use ScopedAllowIO. > They do indirectly use it through TestingProfile but maybe that's it. I think it's alright to do file IO from unit tests[1] hence nobody is using ScopedAllowIO in unittest.cc. [1] Examples: https://cs.chromium.org/search/?q=base::ReadFile+file:unittest.cc$+package:%5... My understanding is that unless you manually call base::ThreadRestrictions::SetIOAllowed(false), file IO is allowed on a thread. This is done for UI and IO threads manually: https://cs.chromium.org/chromium/src/content/browser/browser_process_sub_thre... https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?typ...
satorux@chromium.org changed reviewers: + jochen@chromium.org
ping. +jochen in case dpranke is not available.
lgtm
The CQ bit was checked by satorux@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2911153002/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496298541273730,
"parent_rev": "2150a498ab8ba74261d142941533992fb21bd2d8", "commit_rev":
"e1396f8adefd2f71ed33732f5ffe013bcf87990a"}
Message was sent while issue was closed.
Description was changed from ========== Allow use of ScopedAllowIO in browser tests This change is based on the discussion on a chormium-dev thread titled "ScopedAllowIO ban isn't preventing usage, it's just annoying people" where jam@ mentioned "Excluding test files seems like a good change to me." TEST=tested locally by adding ScopedAllowIO to a random browser test file with and without this change ========== to ========== Allow use of ScopedAllowIO in browser tests This change is based on the discussion on a chormium-dev thread titled "ScopedAllowIO ban isn't preventing usage, it's just annoying people" where jam@ mentioned "Excluding test files seems like a good change to me." TEST=tested locally by adding ScopedAllowIO to a random browser test file with and without this change Review-Url: https://codereview.chromium.org/2911153002 Cr-Commit-Position: refs/heads/master@{#476211} Committed: https://chromium.googlesource.com/chromium/src/+/e1396f8adefd2f71ed33732f5ffe... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e1396f8adefd2f71ed33732f5ffe... |
