Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2876153002:
Support Using ScopedFeatureList in BrowserTest (Closed)
DescriptionSupport Using ScopedFeatureList in SetUp for Browser Test
Currently we don't support using ScopedFeatureList::* in SetUp for InProcessBrowserTest since we clear FeatureList at the beginning of InProcessBrowserTest::SetUp.
In this patch, we remove the FeatureList::ClearInstanceForTesting() and move the feature change in InProcessBrowserTest to ScopedFeatureList. So we are able to move all feature list change calls SetUp and use ScopedFeatureList::*.
BUG=713390
Review-Url: https://codereview.chromium.org/2876153002
Cr-Commit-Position: refs/heads/master@{#485096}
Committed: https://chromium.googlesource.com/chromium/src/+/83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b
Patch Set 1 #Patch Set 2 : fix for windows #
Total comments: 1
Patch Set 3 : use ScopedCommandLine in BrowserTest #Patch Set 4 : add test and init FeatureList with command line in TestSuite::Initialize #
Total comments: 8
Patch Set 5 : update and add comment #Patch Set 6 : ilya's comments addressed #
Total comments: 14
Patch Set 7 : Ilya comment addressed #
Total comments: 13
Patch Set 8 : ilya comments addressed #
Total comments: 8
Patch Set 9 : addressed some comments #Patch Set 10 : fix BrowserTestTest #
Total comments: 1
Patch Set 11 : revert changes in commandline #
Total comments: 3
Patch Set 12 : leave tests and changes in test_suites to following patch #
Depends on Patchset: Messages
Total messages: 104 (74 generated)
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Not For Review] Support Using ScopedFeatureList in SetUp for Browser Test BUG=713390 ========== to ========== Support Using ScopedFeatureList in SetUp for Browser Test Currently we have 2 issues in BrowserTest: 1. If we call ScopedFeatureList::* in SetUp for InProcessBrowserTest, the changes will reset InProcessBrowserTest::SetUp. So ScopedFeatureList::* can only call in SetUpCommandLine. 2. In BrowserTestBase::SetUp, we append current features to command line. If we have a conflict changes for ScopedFeatureList::*. It would be a undefined behavior. In this patch, we have 2 changes: 1. Remove base::FeatureList::ClearInstanceForTesting() in InProcessBrowserTest::SetUp. 2. In BrowserTestBase::SetUp, we change AppendSwitchASCII( to ReplaceSwitchASCIIForTest. BUG=713390 ==========
chaopeng@chromium.org changed reviewers: + isherman@chromium.org
Ilya PTAL. Thank you.
Thanks! https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... content/public/test/browser_test_base.cc:244: enabled_features); Hmm, shouldn't we extend the switch, rather than replacing it entirely? That is, if the switch already specifies some values, I thought we wanted to respect those values (unless they were specifically overridden) -- no?
On 2017/05/16 06:43:32, Ilya Sherman wrote: > Thanks! > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > File content/public/test/browser_test_base.cc (right): > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > content/public/test/browser_test_base.cc:244: enabled_features); > Hmm, shouldn't we extend the switch, rather than replacing it entirely? That > is, if the switch already specifies some values, I thought we wanted to respect > those values (unless they were specifically overridden) -- no? Right, we should use ScopedCommandLine not just CommandLine. Updated, PTAL. Thank you.
The CQ bit was checked by chaopeng@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...
On 2017/05/16 18:29:34, chaopeng wrote: > On 2017/05/16 06:43:32, Ilya Sherman wrote: > > Thanks! > > > > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > > File content/public/test/browser_test_base.cc (right): > > > > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > > content/public/test/browser_test_base.cc:244: enabled_features); > > Hmm, shouldn't we extend the switch, rather than replacing it entirely? That > > is, if the switch already specifies some values, I thought we wanted to > respect > > those values (unless they were specifically overridden) -- no? > > Right, we should use ScopedCommandLine not just CommandLine. Updated, PTAL. > Thank you. Sorry, I'm not following how that helps. I believe that your motivation for starting to work on this area was that some bots running tests might explicitly configure a feature -- let's say, they pass --enable-features=WhatAGreatTestingFeature. The expectation is that the great testing feature would be enabled for all tests, unless some test explicitly overrides it being enabled. Right? Does your code do that? If so, how? Perhaps I'm just confused?
On 2017/05/16 19:58:03, Ilya Sherman wrote: > On 2017/05/16 18:29:34, chaopeng wrote: > > On 2017/05/16 06:43:32, Ilya Sherman wrote: > > > Thanks! > > > > > > > > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > > > File content/public/test/browser_test_base.cc (right): > > > > > > > > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > > > content/public/test/browser_test_base.cc:244: enabled_features); > > > Hmm, shouldn't we extend the switch, rather than replacing it entirely? > That > > > is, if the switch already specifies some values, I thought we wanted to > > respect > > > those values (unless they were specifically overridden) -- no? > > > > Right, we should use ScopedCommandLine not just CommandLine. Updated, PTAL. > > Thank you. > > Sorry, I'm not following how that helps. I believe that your motivation for > starting to work on this area was that some bots running tests might explicitly > configure a feature -- let's say, they pass > --enable-features=WhatAGreatTestingFeature. The expectation is that the great > testing feature would be enabled for all tests, unless some test explicitly > overrides it being enabled. Right? Does your code do that? If so, how? > Perhaps I'm just confused? No, This patch only is for let BrowserTest handle the FeatureList changes from SetUp and SetUpCommandLine correctly. After this patch I will reland crrev.com/2834583002
On 2017/05/16 20:16:30, chaopeng wrote: > On 2017/05/16 19:58:03, Ilya Sherman wrote: > > On 2017/05/16 18:29:34, chaopeng wrote: > > > On 2017/05/16 06:43:32, Ilya Sherman wrote: > > > > Thanks! > > > > > > > > > > > > > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > > > > File content/public/test/browser_test_base.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > > > > content/public/test/browser_test_base.cc:244: enabled_features); > > > > Hmm, shouldn't we extend the switch, rather than replacing it entirely? > > That > > > > is, if the switch already specifies some values, I thought we wanted to > > > respect > > > > those values (unless they were specifically overridden) -- no? > > > > > > Right, we should use ScopedCommandLine not just CommandLine. Updated, PTAL. > > > Thank you. > > > > Sorry, I'm not following how that helps. I believe that your motivation for > > starting to work on this area was that some bots running tests might > explicitly > > configure a feature -- let's say, they pass > > --enable-features=WhatAGreatTestingFeature. The expectation is that the great > > testing feature would be enabled for all tests, unless some test explicitly > > overrides it being enabled. Right? Does your code do that? If so, how? > > Perhaps I'm just confused? > > No, This patch only is for let BrowserTest handle the FeatureList changes from > SetUp and SetUpCommandLine correctly. After this patch I will reland > crrev.com/2834583002 Do you plan to update browser_test_base.cc again when relanding that CL? If not, how are you going to avoid losing the command line state configured prior to the tests running?
On 2017/05/16 20:18:45, Ilya Sherman wrote: > On 2017/05/16 20:16:30, chaopeng wrote: > > On 2017/05/16 19:58:03, Ilya Sherman wrote: > > > On 2017/05/16 18:29:34, chaopeng wrote: > > > > On 2017/05/16 06:43:32, Ilya Sherman wrote: > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > > > > > File content/public/test/browser_test_base.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2876153002/diff/20001/content/public/test/bro... > > > > > content/public/test/browser_test_base.cc:244: enabled_features); > > > > > Hmm, shouldn't we extend the switch, rather than replacing it entirely? > > > That > > > > > is, if the switch already specifies some values, I thought we wanted to > > > > respect > > > > > those values (unless they were specifically overridden) -- no? > > > > > > > > Right, we should use ScopedCommandLine not just CommandLine. Updated, > PTAL. > > > > Thank you. > > > > > > Sorry, I'm not following how that helps. I believe that your motivation for > > > starting to work on this area was that some bots running tests might > > explicitly > > > configure a feature -- let's say, they pass > > > --enable-features=WhatAGreatTestingFeature. The expectation is that the > great > > > testing feature would be enabled for all tests, unless some test explicitly > > > overrides it being enabled. Right? Does your code do that? If so, how? > > > Perhaps I'm just confused? > > > > No, This patch only is for let BrowserTest handle the FeatureList changes from > > SetUp and SetUpCommandLine correctly. After this patch I will reland > > crrev.com/2834583002 > > Do you plan to update browser_test_base.cc again when relanding that CL? If > not, how are you going to avoid losing the command line state configured prior > to the tests running? Right, I should reland that patch first.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chaopeng@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...
chaopeng@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Ilya, PTAL. Thank you.
Thanks for continuing to work on this! https://codereview.chromium.org/2876153002/diff/60001/base/command_line.h File base/command_line.h (right): https://codereview.chromium.org/2876153002/diff/60001/base/command_line.h#new... base/command_line.h:186: const std::string& value); I happened to find this code [1] in a code search. It looks like it's trying to do approximately the same thing you're doing. Can we unify the code paths? [1] https://cs.chromium.org/chromium/src/android_webview/browser/command_line_hel... https://codereview.chromium.org/2876153002/diff/60001/base/test/test_suite.cc File base/test/test_suite.cc (right): https://codereview.chromium.org/2876153002/diff/60001/base/test/test_suite.cc... base/test/test_suite.cc:349: created_feature_list_ = FeatureList::InitializeInstance( Could you use a ScopedFeatureList here? https://codereview.chromium.org/2876153002/diff/60001/base/test/test_suite.cc... base/test/test_suite.cc:352: "disable-features")); nit: Please move the kEnabledFeatures and kDisabledFeatures constants into //base, and use them here. https://codereview.chromium.org/2876153002/diff/60001/base/test/test_suite.cc... base/test/test_suite.cc:352: "disable-features")); It looks like the code expects that sometimes this InitializeInstance call might fail. That seems wrong. Are there currently cases where it fails? Can we change the code to avoid the possibility of failure, and add an assertion to that effect? https://codereview.chromium.org/2876153002/diff/60001/content/public/test/bro... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/2876153002/diff/60001/content/public/test/bro... content/public/test/browser_test_base.cc:155: base::test::ScopedCommandLine scoped_command_line; I don't think it should be necessary to use a ScopedCommandLine here. Does anything go wrong if you modify the original command-line rather than a scoped copy? https://codereview.chromium.org/2876153002/diff/60001/content/public/test/bro... content/public/test/browser_test_base.cc:247: enabled_features); I don't think that it's appropriate to just replace whatever switches were already set by this point. Instead, I think we should *remove* the switches when they are read into the FeatureList in //base/test/test_suite; and we should assert here that there are no --enable-features or --disable-features switches present. Otherwise, if some code tried to enable/disable features this way between when the //base/test/test_suite code ran and when this code runs, it'll silently fail, which is not good.
https://codereview.chromium.org/2876153002/diff/60001/base/command_line.h File base/command_line.h (right): https://codereview.chromium.org/2876153002/diff/60001/base/command_line.h#new... base/command_line.h:186: const std::string& value); On 2017/05/25 22:48:11, Ilya Sherman wrote: > I happened to find this code [1] in a code search. It looks like it's trying to > do approximately the same thing you're doing. Can we unify the code paths? > > [1] > https://cs.chromium.org/chromium/src/android_webview/browser/command_line_hel... CommandLineHelper try to feature only if feature not in enable or disable list. ScopedFeatureList want to override feature. But I want to change CommandLineHelper to call ReplaceSwitchASCII. https://codereview.chromium.org/2876153002/diff/60001/content/public/test/bro... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/2876153002/diff/60001/content/public/test/bro... content/public/test/browser_test_base.cc:247: enabled_features); On 2017/05/25 22:48:12, Ilya Sherman wrote: > I don't think that it's appropriate to just replace whatever switches were > already set by this point. Instead, I think we should *remove* the switches > when they are read into the FeatureList in //base/test/test_suite; and we should > assert here that there are no --enable-features or --disable-features switches > present. Otherwise, if some code tried to enable/disable features this way > between when the //base/test/test_suite code ran and when this code runs, it'll > silently fail, which is not good. This is good to prevent teams using commandline to change features in SetUp. Maybe I need a patch to fix that first.
Hi Jianpeng, just wanted to check in: What's the status of this CL?
On 2017/06/05 21:52:04, Ilya Sherman wrote: > Hi Jianpeng, just wanted to check in: What's the status of this CL? I am working on a patch to move all command line call to ScopedFeatureList.
Updated, PTAL. Thank you.
Thanks for continuing to work on this! I think it's getting quite close =) https://codereview.chromium.org/2876153002/diff/100001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2876153002/diff/100001/base/command_line.cc#n... base/command_line.cc:376: #endif These few lines are duplicated from above. Please factor out a small helper function for this rather than copy/pasting them. https://codereview.chromium.org/2876153002/diff/100001/base/command_line.cc#n... base/command_line.cc:379: switches_.erase(switch_string.substr(prefix_length)); Does argv_ need to be updated as well? (I am not familiar with the details of the CommandLine implementation, so I'm just judging by the fact that it's set in AppendSwitchNative().) https://codereview.chromium.org/2876153002/diff/100001/base/command_line.h File base/command_line.h (right): https://codereview.chromium.org/2876153002/diff/100001/base/command_line.h#ne... base/command_line.h:185: void RemoveSwitchASCIIForTesting(const std::string& switch_string); nit: Please drop "ASCII" from this name -- that refers to the value, and this does not consider the value. https://codereview.chromium.org/2876153002/diff/100001/base/command_line.h#ne... base/command_line.h:185: void RemoveSwitchASCIIForTesting(const std::string& switch_string); nit: Please separate this from the lines above with a blank line of whitespace, and please document this function. https://codereview.chromium.org/2876153002/diff/100001/base/test/test_suite.cc File base/test/test_suite.cc (right): https://codereview.chromium.org/2876153002/diff/100001/base/test/test_suite.c... base/test/test_suite.cc:345: // an error that it's not set. it will be cleared automatically. nit: s/it/It https://codereview.chromium.org/2876153002/diff/100001/base/test/test_suite.c... base/test/test_suite.cc:352: // FeatureList nit: It's a bit odd to mention BrowserTestBase from this file, since that file depends on this one. I'd replace this comment with something like: // The enable-features and disable-features flags were just slurped into a // FeatureList, so remove them from the command line. Tests should enable and // disable features via the ScopedFeatureList API rather than command-line // flags. https://codereview.chromium.org/2876153002/diff/100001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/100001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test_browsertest.cc:158: "TestFeatureForBrowserTest", base::FEATURE_DISABLED_BY_DEFAULT}; nit: Please define this in an anonymous namespace. https://codereview.chromium.org/2876153002/diff/100001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test_browsertest.cc:158: "TestFeatureForBrowserTest", base::FEATURE_DISABLED_BY_DEFAULT}; Is it worth having both an enabled and a disabled feature get tested? https://codereview.chromium.org/2876153002/diff/100001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test_browsertest.cc:167: command_line->GetSwitchValueASCII(switches::kEnableFeatures); Same questions apply here as for the other test. In fact, is it really necessary to have both? I'd expect them to provide roughly equivalent coverage to one another. https://codereview.chromium.org/2876153002/diff/100001/content/test/content_b... File content/test/content_browser_test_test.cc (right): https://codereview.chromium.org/2876153002/diff/100001/content/test/content_b... content/test/content_browser_test_test.cc:182: "TestFeatureForContentBrowserTest", base::FEATURE_DISABLED_BY_DEFAULT}; nit: Please define this in an anonymous namespace. https://codereview.chromium.org/2876153002/diff/100001/content/test/content_b... content/test/content_browser_test_test.cc:182: "TestFeatureForContentBrowserTest", base::FEATURE_DISABLED_BY_DEFAULT}; Is it worth having both an enabled and a disabled feature get tested? https://codereview.chromium.org/2876153002/diff/100001/content/test/content_b... content/test/content_browser_test_test.cc:191: command_line->GetSwitchValueASCII(switches::kEnableFeatures); This should probably be kDisableFeatures... though I'm actually not sure why you're saving these at all. If you don't control them within the test, why are you including them in what's tested? https://codereview.chromium.org/2876153002/diff/100001/content/test/content_b... content/test/content_browser_test_test.cc:215: // Ensure we repected the features from command line. Is there reason to expect that there *are* features on the command line? It would be more compelling if you added features to the command-line as part of the test.
Patchset #7 (id:120001) has been deleted
Updated. PTAL. Thank you. https://codereview.chromium.org/2876153002/diff/100001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/100001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test_browsertest.cc:158: "TestFeatureForBrowserTest", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/06/07 21:32:36, Ilya Sherman wrote: > nit: Please define this in an anonymous namespace. This file is already in anonymous.
https://codereview.chromium.org/2876153002/diff/140001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2876153002/diff/140001/base/command_line.cc#n... base/command_line.cc:152: std::string SwitchKey(const std::string& switch_string) { nit: Mebbe "GetSwitchKey"? https://codereview.chromium.org/2876153002/diff/140001/base/command_line.cc#n... base/command_line.cc:160: CommandLine::StringType CombinedSwitchString(const std::string switch_key) { nit: Mebbe "GetCombinedSwitchString"? https://codereview.chromium.org/2876153002/diff/140001/base/command_line.cc#n... base/command_line.cc:387: auto find_switch = [&combined_switch_string](const std::string& s) { nit: What does "s" represent? Please give it a clearer name, e.g. "argv_entry". https://codereview.chromium.org/2876153002/diff/140001/base/command_line.h File base/command_line.h (right): https://codereview.chromium.org/2876153002/diff/140001/base/command_line.h#ne... base/command_line.h:186: void RemoveSwitchForTesting(const std::string& switch_string); Please document this function. https://codereview.chromium.org/2876153002/diff/140001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2876153002/diff/140001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:60: } // namespace features Why did you move this out of the chrome_browser_net namespace? Should this actually be defined in https://cs.chromium.org/chromium/src/chrome/common/chrome_features.h ? https://codereview.chromium.org/2876153002/diff/140001/chrome/browser/process... File chrome/browser/process_singleton_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/140001/chrome/browser/process... chrome/browser/process_singleton_browsertest.cc:87: "NetworkPrediction"); Is this actually necessary? I don't think this test uses the Python webserver -- does it? https://codereview.chromium.org/2876153002/diff/140001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/140001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test_browsertest.cc:188: base::SPLIT_WANT_NONEMPTY); Again, if this test doesn't know what features were enabled on the command-line, why is it testing them? It's plausible to me that there simply aren't any enabled features, and this is a no-op. If you want to test this behavior, please test it in a way where you are controlling at least one feature that is enabled. https://codereview.chromium.org/2876153002/diff/140001/chrome/test/base/test_... File chrome/test/base/test_launcher_utils.cc (right): https://codereview.chromium.org/2876153002/diff/140001/chrome/test/base/test_... chrome/test/base/test_launcher_utils.cc:86: } ^^^ looks like what you are trying to implement in the CommandLine changes. https://codereview.chromium.org/2876153002/diff/140001/content/test/content_b... File content/test/content_browser_test_test.cc (right): https://codereview.chromium.org/2876153002/diff/140001/content/test/content_b... content/test/content_browser_test_test.cc:183: const base::Feature kTestFeatureForContentBrowserTest{ nit: Please add an extra space before the curly brace. https://codereview.chromium.org/2876153002/diff/140001/content/test/content_b... content/test/content_browser_test_test.cc:223: EXPECT_TRUE(base::FeatureList::IsEnabled(kTestFeatureForContentBrowserTest)); I don't understand. Where is the feature being enabled?
Hi Ilya, I am thinking to split this to two CL. 1. Support using ScopedFeatureList in BrowserTest::SetUp. 2. Forbid calling CommandLine::* to modify features. I am not enable to run dry run since this would fail because lot CommandLine::* calls modify features. And the following patch can modify features in SetUp instead of SetUpCommandLine. WDYT? Also this patch updated. PTAL. Thank you. https://codereview.chromium.org/2876153002/diff/140001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/140001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test_browsertest.cc:188: base::SPLIT_WANT_NONEMPTY); On 2017/06/09 20:19:28, Ilya Sherman wrote: > Again, if this test doesn't know what features were enabled on the command-line, > why is it testing them? It's plausible to me that there simply aren't any > enabled features, and this is a no-op. If you want to test this behavior, > please test it in a way where you are controlling at least one feature that is > enabled. These two tests seems useless, maybe I should just remove it. https://codereview.chromium.org/2876153002/diff/140001/chrome/test/base/test_... File chrome/test/base/test_launcher_utils.cc (right): https://codereview.chromium.org/2876153002/diff/140001/chrome/test/base/test_... chrome/test/base/test_launcher_utils.cc:86: } On 2017/06/09 20:19:28, Ilya Sherman wrote: > ^^^ looks like what you are trying to implement in the CommandLine changes. Yes, we should remove this method after. https://codereview.chromium.org/2876153002/diff/140001/content/test/content_b... File content/test/content_browser_test_test.cc (right): https://codereview.chromium.org/2876153002/diff/140001/content/test/content_b... content/test/content_browser_test_test.cc:223: EXPECT_TRUE(base::FeatureList::IsEnabled(kTestFeatureForContentBrowserTest)); On 2017/06/09 20:19:28, Ilya Sherman wrote: > I don't understand. Where is the feature being enabled? Sorry, Forgot to test it before send to you.
Patchset #8 (id:160001) has been deleted
On 2017/06/13 04:16:32, chaopeng wrote: > Hi Ilya, I am thinking to split this to two CL. > > 1. Support using ScopedFeatureList in BrowserTest::SetUp. > 2. Forbid calling CommandLine::* to modify features. > > I am not enable to run dry run since this would fail because lot CommandLine::* > calls modify features. And the following patch can modify features in SetUp > instead of SetUpCommandLine. WDYT? Splitting this into two CLs sounds okay to me, thanks. > Also this patch updated. PTAL. Thank you. Thanks. For future reference: Could you please tap the "Done" button when you address comments? It's helpful for reviewers to keep better track of what has been addressed. https://codereview.chromium.org/2876153002/diff/180001/chrome/browser/process... File chrome/browser/process_singleton_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/180001/chrome/browser/process... chrome/browser/process_singleton_browsertest.cc:38: #include "content/public/common/content_switches.h" Please revert the changes to this file. https://codereview.chromium.org/2876153002/diff/180001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2876153002/diff/180001/chrome/common/chrome_f... chrome/common/chrome_features.h:205: extern const base::Feature kNetworkPrediction; nit: Please try to keep this file alphabetized (see the comment at the bottom of the file). https://codereview.chromium.org/2876153002/diff/180001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/180001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test_browsertest.cc:195: EXPECT_TRUE(base::FeatureList::IsEnabled(kTestFeatureForBrowserTest)); IMO this part of the test is useful. The EXPECT_NE in the loop above is not as useful, since this test doesn't actually control what features are enabled/disabled prior to the test; and it might in fact be the empty set. It certainly doesn't make much sense to test *only* the enabled features, and not the disabled features as well. We should either test both or neither. https://codereview.chromium.org/2876153002/diff/180001/content/public/test/br... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/2876153002/diff/180001/content/public/test/br... content/public/test/browser_test_base.cc:234: << "Should not use CommandLine to modify features."; Please list this as a comment if you'd like to include it. Outputting strings with DCHECKs adds weight to the binary, and hence is only recommended when outputting non-static data, e.g. a filename.
perchdaddy85@gmail.com changed reviewers: + perchdaddy85@gmail.com
The CQ bit was checked by chaopeng@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...
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #11 (id:280001) has been deleted
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:300001) has been deleted
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Patchset #10 (id:320001) has been deleted
The CQ bit was checked by chaopeng@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...
Ilya, sorry for a long wait. PTAL. Thank you. https://codereview.chromium.org/2876153002/diff/180001/chrome/browser/process... File chrome/browser/process_singleton_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/180001/chrome/browser/process... chrome/browser/process_singleton_browsertest.cc:38: #include "content/public/common/content_switches.h" On 2017/06/13 22:42:48, Ilya Sherman wrote: > Please revert the changes to this file. Done. https://codereview.chromium.org/2876153002/diff/180001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2876153002/diff/180001/chrome/common/chrome_f... chrome/common/chrome_features.h:205: extern const base::Feature kNetworkPrediction; On 2017/06/13 22:42:48, Ilya Sherman wrote: > nit: Please try to keep this file alphabetized (see the comment at the bottom of > the file). Done. https://codereview.chromium.org/2876153002/diff/180001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/2876153002/diff/180001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test_browsertest.cc:195: EXPECT_TRUE(base::FeatureList::IsEnabled(kTestFeatureForBrowserTest)); On 2017/06/13 22:42:48, Ilya Sherman wrote: > IMO this part of the test is useful. The EXPECT_NE in the loop above is not as > useful, since this test doesn't actually control what features are > enabled/disabled prior to the test; and it might in fact be the empty set. It > certainly doesn't make much sense to test *only* the enabled features, and not > the disabled features as well. We should either test both or neither. Done. https://codereview.chromium.org/2876153002/diff/180001/content/public/test/br... File content/public/test/browser_test_base.cc (right): https://codereview.chromium.org/2876153002/diff/180001/content/public/test/br... content/public/test/browser_test_base.cc:234: << "Should not use CommandLine to modify features."; On 2017/06/13 22:42:48, Ilya Sherman wrote: > Please list this as a comment if you'd like to include it. Outputting strings > with DCHECKs adds weight to the binary, and hence is only recommended when > outputting non-static data, e.g. a filename. Done. https://codereview.chromium.org/2876153002/diff/340001/base/test/test_suite.cc File base/test/test_suite.cc (right): https://codereview.chromium.org/2876153002/diff/340001/base/test/test_suite.c... base/test/test_suite.cc:353: enabled += ",TestFeatureForBrowserTest1"; Here is the only correct timing can change features by command line.
The CQ bit was unchecked by chaopeng@chromium.org
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks! https://codereview.chromium.org/2876153002/diff/360001/base/test/test_suite.cc File base/test/test_suite.cc (right): https://codereview.chromium.org/2876153002/diff/360001/base/test/test_suite.c... base/test/test_suite.cc:354: disabled += ",TestFeatureForBrowserTest2"; Let's omit these for now, and perhaps add them in a follow-up CL. (Please make sure to update the comment above as well.)
https://codereview.chromium.org/2876153002/diff/360001/base/test/test_suite.cc File base/test/test_suite.cc (right): https://codereview.chromium.org/2876153002/diff/360001/base/test/test_suite.c... base/test/test_suite.cc:354: disabled += ",TestFeatureForBrowserTest2"; On 2017/06/28 16:20:20, Ilya Sherman wrote: > Let's omit these for now, and perhaps add them in a follow-up CL. (Please make > sure to update the comment above as well.) But BrowserTestScopedFeatureListTest need this features for testing.
The CQ bit was checked by chaopeng@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 ========== Support Using ScopedFeatureList in SetUp for Browser Test Currently we have 2 issues in BrowserTest: 1. If we call ScopedFeatureList::* in SetUp for InProcessBrowserTest, the changes will reset InProcessBrowserTest::SetUp. So ScopedFeatureList::* can only call in SetUpCommandLine. 2. In BrowserTestBase::SetUp, we append current features to command line. If we have a conflict changes for ScopedFeatureList::*. It would be a undefined behavior. In this patch, we have 2 changes: 1. Remove base::FeatureList::ClearInstanceForTesting() in InProcessBrowserTest::SetUp. 2. In BrowserTestBase::SetUp, we change AppendSwitchASCII( to ReplaceSwitchASCIIForTest. BUG=713390 ========== to ========== Support Using ScopedFeatureList in SetUp for Browser Test Currently we don't support using ScopedFeatureList::* in SetUp for InProcessBrowserTest since we clear FeatureList at the beginning of InProcessBrowserTest::SetUp. In this patch, we remove the FeatureList::ClearInstanceForTesting() and move the feature change in InProcessBrowserTest to ScopedFeatureList. So we are able to move all feature list change calls SetUp and use ScopedFeatureList::*. BUG=713390 ==========
Commit message updated. Also I omit the test_suites and 2 browser tests to following patches. https://codereview.chromium.org/2876153002/diff/360001/base/test/test_suite.cc File base/test/test_suite.cc (right): https://codereview.chromium.org/2876153002/diff/360001/base/test/test_suite.c... base/test/test_suite.cc:354: disabled += ",TestFeatureForBrowserTest2"; On 2017/06/28 16:20:20, Ilya Sherman wrote: > Let's omit these for now, and perhaps add them in a follow-up CL. (Please make > sure to update the comment above as well.) I need to revert this change to load empty string to feature list, since not empty string will forbid using commandline to change feature list. Need the following patches to move to ScopedFeatureList first.
LGTM, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
thestig@chromium.org changed reviewers: - perchdaddy85@gmail.com
lgtm
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chaopeng@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by chaopeng@chromium.org
The CQ bit was checked by chaopeng@chromium.org
The CQ bit was unchecked by chaopeng@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chaopeng@chromium.org
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": 380001, "attempt_start_ts": 1499462296778050,
"parent_rev": "669158e6acfd160a5a5ada8c12c43ef22016d1cb", "commit_rev":
"83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b"}
Message was sent while issue was closed.
Description was changed from ========== Support Using ScopedFeatureList in SetUp for Browser Test Currently we don't support using ScopedFeatureList::* in SetUp for InProcessBrowserTest since we clear FeatureList at the beginning of InProcessBrowserTest::SetUp. In this patch, we remove the FeatureList::ClearInstanceForTesting() and move the feature change in InProcessBrowserTest to ScopedFeatureList. So we are able to move all feature list change calls SetUp and use ScopedFeatureList::*. BUG=713390 ========== to ========== Support Using ScopedFeatureList in SetUp for Browser Test Currently we don't support using ScopedFeatureList::* in SetUp for InProcessBrowserTest since we clear FeatureList at the beginning of InProcessBrowserTest::SetUp. In this patch, we remove the FeatureList::ClearInstanceForTesting() and move the feature change in InProcessBrowserTest to ScopedFeatureList. So we are able to move all feature list change calls SetUp and use ScopedFeatureList::*. BUG=713390 Review-Url: https://codereview.chromium.org/2876153002 Cr-Commit-Position: refs/heads/master@{#485096} Committed: https://chromium.googlesource.com/chromium/src/+/83b3cdfd66d9ce73d5f4a8a0d498... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:380001) as https://chromium.googlesource.com/chromium/src/+/83b3cdfd66d9ce73d5f4a8a0d498... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
