Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1141163002:
Browser tests for the new way of handling device ID in Chrome OS. (Closed)
|
Created:
5 years, 7 months ago by dzhioev (left Google) Modified:
5 years, 6 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@device_id Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBrowser tests for the new way of handling device ID in Chrome OS.
BUG=486044, 486136
TEST=browser_tests --gtest_filter=DeviceIDTest.*
Committed: https://crrev.com/1e76fed394f8a90b049ce2c06ca06f677c899024
Cr-Commit-Position: refs/heads/master@{#331828}
Patch Set 1 #Patch Set 2 : Fixed flakiness. #
Total comments: 37
Patch Set 3 : Comments addressed. #Patch Set 4 : OobeBaseTest part extracted. #
Total comments: 16
Patch Set 5 : Comments addressed. #
Messages
Total messages: 39 (13 generated)
dzhioev@chromium.org changed reviewers: + achuith@chromium.org, rogerta@chromium.org
Hello, This CL contains browser tests for CL https://codereview.chromium.org/1138143002 Please review Roger: fake_gaia.* Achuith: the rest.
The CQ bit was checked by dzhioev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141163002/1
The CQ bit was unchecked by dzhioev@chromium.org
nkostylev@chromium.org changed reviewers: + nkostylev@chromium.org
Seems that test fails.
On 2015/05/15 12:17:07, Nikita wrote: > Seems that test fails. Test fails because CL https://codereview.chromium.org/1138143002 is not committed yet =)
The CQ bit was checked by dzhioev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141163002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
nkostylev@chromium.org changed reviewers: - nkostylev@chromium.org
Now that original CL has langed, makes sense to re-run bots and see that test passes.
The CQ bit was checked by dzhioev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141163002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dzhioev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141163002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I fixed a flakiness, now CL passes CQ. PTAL
google_apis lgtm
I feel like the changes to oobe_base_test should be pulled out into a separate CL. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/device_id_browsertest.cc (right): https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:29: const base::FilePath::CharType kRefreshTokenToDeviceIdMapPath[] = kRefreshTokenToDeviceIdMapFile or kRefreshTokenToDeviceIdMapFilename? https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:80: std::string device_id_in_signin_client = nit: const https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:82: std::string device_id_in_local_state = GetDeviceId(user_id); nit: const https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:88: std::string device_id_in_gaia = GetDeviceIdFromGAIA(refresh_token); nit: const https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:112: CheckDeviceIDIsConsistent(user_id, refresh_token); I feel you should pull this function out and explicitly call it from the tests instead of having it be a side effect of signing in. I understand that this will add some verbosity to the tests. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:125: CheckDeviceIDIsConsistent(user_id, refresh_token); Same https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:134: void OnBeforeUserRemoved(const std::string& username) override {} // RemoveUserDelegate: https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:141: base::FilePath path = base::CommandLine::ForCurrentProcess() nit: const, also a slightly more descriptive name would be nice here https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:146: return; When is this a valid outcome? https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:151: for (base::DictionaryValue::Iterator it(*dictionary); !it.IsAtEnd(); Why not auto here? https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:166: base::FilePath path = base::CommandLine::ForCurrentProcess() nit: const, also more descriptive name. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:183: std::string device_id = GetDeviceId(kFakeUserEmail); nit: const https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:195: std::string device_id = GetDeviceId(kFakeUserEmail); nit:const https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:227: IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_Migration) { This "test" isn't testing anything, is it? If it's not, shouldn't it just be a method instead of a test? https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/test/oobe_base_test.cc (right): https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/test/oobe_base_test.cc:241: ASSERT_TRUE(content::ExecuteScript( Shouldn't you update this code as well? https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/test/oobe_base_test.h (right): https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/test/oobe_base_test.h:77: test::JSChecker& JS() { return js_checker_; } I think returning a pointer is preferred to a non-const reference here. https://codereview.chromium.org/1141163002/diff/20001/google_apis/gaia/fake_g... File google_apis/gaia/fake_gaia.cc (right): https://codereview.chromium.org/1141163002/diff/20001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.cc:332: auto it = refresh_token_to_device_id_map_.find(refresh_token); const? Not sure if it works https://codereview.chromium.org/1141163002/diff/20001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.cc:337: void FakeGaia::SetRefreshTokenToDeviceIdMap( Shouldn't this be set_refresh_token_to_device_id_map(), with the inline implementation in the .h file?
https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/device_id_browsertest.cc (right): https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:29: const base::FilePath::CharType kRefreshTokenToDeviceIdMapPath[] = On 2015/05/26 06:28:20, achuithb wrote: > kRefreshTokenToDeviceIdMapFile or kRefreshTokenToDeviceIdMapFilename? Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:80: std::string device_id_in_signin_client = On 2015/05/26 06:28:20, achuithb wrote: > nit: const Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:82: std::string device_id_in_local_state = GetDeviceId(user_id); On 2015/05/26 06:28:20, achuithb wrote: > nit: const Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:88: std::string device_id_in_gaia = GetDeviceIdFromGAIA(refresh_token); On 2015/05/26 06:28:20, achuithb wrote: > nit: const Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:112: CheckDeviceIDIsConsistent(user_id, refresh_token); On 2015/05/26 06:28:20, achuithb wrote: > I feel you should pull this function out and explicitly call it from the tests > instead of having it be a side effect of signing in. I understand that this will > add some verbosity to the tests. Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:125: CheckDeviceIDIsConsistent(user_id, refresh_token); On 2015/05/26 06:28:20, achuithb wrote: > Same Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:134: void OnBeforeUserRemoved(const std::string& username) override {} On 2015/05/26 06:28:19, achuithb wrote: > // RemoveUserDelegate: Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:141: base::FilePath path = base::CommandLine::ForCurrentProcess() On 2015/05/26 06:28:20, achuithb wrote: > nit: const, also a slightly more descriptive name would be nice here Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:146: return; On 2015/05/26 06:28:20, achuithb wrote: > When is this a valid outcome? When the file doesn't exist. This happens in the very first test in PRE_* tests chain, when data dir is empty. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:151: for (base::DictionaryValue::Iterator it(*dictionary); !it.IsAtEnd(); On 2015/05/26 06:28:20, achuithb wrote: > Why not auto here? base::Dictionary doesn't have begin() method returning an iterator, as standard containers do. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:166: base::FilePath path = base::CommandLine::ForCurrentProcess() On 2015/05/26 06:28:20, achuithb wrote: > nit: const, also more descriptive name. Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:183: std::string device_id = GetDeviceId(kFakeUserEmail); On 2015/05/26 06:28:20, achuithb wrote: > nit: const Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:195: std::string device_id = GetDeviceId(kFakeUserEmail); On 2015/05/26 06:28:20, achuithb wrote: > nit:const Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:227: IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_Migration) { On 2015/05/26 06:28:20, achuithb wrote: > This "test" isn't testing anything, is it? > > If it's not, shouldn't it just be a method instead of a test? Yes, that is PRE test case where we set up the state for the real test case (DeviceIDTest.Migration). User can't "sign out" on Chrome OS without restarting a browser process, and the only way to keep the user data dir between browser restarts in browser tests is a PRE_* test. I reworded comments a little to make it clear what happens in each test case. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/test/oobe_base_test.cc (right): https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/test/oobe_base_test.cc:241: ASSERT_TRUE(content::ExecuteScript( On 2015/05/26 06:28:21, achuithb wrote: > Shouldn't you update this code as well? Done. https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/test/oobe_base_test.h (right): https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/test/oobe_base_test.h:77: test::JSChecker& JS() { return js_checker_; } On 2015/05/26 06:28:21, achuithb wrote: > I think returning a pointer is preferred to a non-const reference here. Why? I don't see any advantage in that. https://codereview.chromium.org/1141163002/diff/20001/google_apis/gaia/fake_g... File google_apis/gaia/fake_gaia.cc (right): https://codereview.chromium.org/1141163002/diff/20001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.cc:332: auto it = refresh_token_to_device_id_map_.find(refresh_token); On 2015/05/26 06:28:21, achuithb wrote: > const? Not sure if it works Done. https://codereview.chromium.org/1141163002/diff/20001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.cc:337: void FakeGaia::SetRefreshTokenToDeviceIdMap( On 2015/05/26 06:28:21, achuithb wrote: > Shouldn't this be set_refresh_token_to_device_id_map(), with the inline > implementation in the .h file? Inline setters are only allowed for trivial types.
https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/test/oobe_base_test.h (right): https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/test/oobe_base_test.h:77: test::JSChecker& JS() { return js_checker_; } On 2015/05/26 21:18:15, dzhioev wrote: > On 2015/05/26 06:28:21, achuithb wrote: > > I think returning a pointer is preferred to a non-const reference here. > > Why? I don't see any advantage in that. This is for arguments, but I think the logic also holds for return values: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... If you have strong feelings about this, I'm fine with returning non-const reference. I'm sure there are plenty of places in the codebase that do exactly this.
On 2015/05/26 06:28:21, achuithb wrote: > I feel like the changes to oobe_base_test should be pulled out into a separate > CL. > ping? My rationale is that if the rest of this CL gets reverted due to test failures/flakes, this part should stay landed.
On 2015/05/26 21:26:44, achuithb wrote: > https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/test/oobe_base_test.h (right): > > https://codereview.chromium.org/1141163002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/login/test/oobe_base_test.h:77: test::JSChecker& JS() { > return js_checker_; } > On 2015/05/26 21:18:15, dzhioev wrote: > > On 2015/05/26 06:28:21, achuithb wrote: > > > I think returning a pointer is preferred to a non-const reference here. > > > > Why? I don't see any advantage in that. > > This is for arguments, but I think the logic also holds for return values: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... > > If you have strong feelings about this, I'm fine with returning non-const > reference. I'm sure there are plenty of places in the codebase that do exactly > this. I think the rationale for reference arguments doesn't work here. The worst thing that can happen is that code won't compile. I believe returning a reference is even safer than returning a pointer, as it makes harder for user to save a returned value and use it after an owning object destruction. not copyable.
On 2015/05/26 21:28:26, achuithb wrote: > On 2015/05/26 06:28:21, achuithb wrote: > > I feel like the changes to oobe_base_test should be pulled out into a separate > > CL. > > > > ping? > > My rationale is that if the rest of this CL gets reverted due to test > failures/flakes, this part should stay landed. OK, I'll split this CL.
On 2015/05/27 23:24:01, dzhioev wrote: > On 2015/05/26 21:28:26, achuithb wrote: > > On 2015/05/26 06:28:21, achuithb wrote: > > > I feel like the changes to oobe_base_test should be pulled out into a > separate > > > CL. > > > > > > > ping? > > > > My rationale is that if the rest of this CL gets reverted due to test > > failures/flakes, this part should stay landed. > > OK, I'll split this CL. Done.
https://codereview.chromium.org/1141163002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/device_id_browsertest.cc (right): https://codereview.chromium.org/1141163002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:159: void SaveRefreshTokenToDeviceIdMap() const { nit: I'd remove the const here; the function is not logically const and this doesn't add any value https://codereview.chromium.org/1141163002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:227: // Set up an user that has device ID stored in preference only. // Set up a user that has a device ID stored in preferences only. https://codereview.chromium.org/1141163002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:253: // Set up an user that doesn't have device ID. // Set up a user that doesn't have a device ID. https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... File google_apis/gaia/fake_gaia.cc (right): https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.cc:140: auto maybe_update_field = The number of obscure C++ features used in this snippet of code is pretty mind-boggling, but I guess this is ok. Could you add a comment like // This lambda uses a pointer to data member to merge attributes. https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... File google_apis/gaia/fake_gaia.h (right): https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.h:35: typedef std::map<std::string, std::string> RefreshTokenToDeviceIdMap; Perhaps replace both typedefs with using? https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.h:57: void Update(const MergeSessionParams& update); call args params for consistency? https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.h:93: // Updates various params with non-empty values from |update|. |params| https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.h:133: // Returns a device ID associated with a given refresh token. |refresh_token|
https://codereview.chromium.org/1141163002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/device_id_browsertest.cc (right): https://codereview.chromium.org/1141163002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:159: void SaveRefreshTokenToDeviceIdMap() const { On 2015/05/28 00:18:57, achuithb wrote: > nit: I'd remove the const here; the function is not logically const and this > doesn't add any value Done. https://codereview.chromium.org/1141163002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:227: // Set up an user that has device ID stored in preference only. On 2015/05/28 00:18:57, achuithb wrote: > // Set up a user that has a device ID stored in preferences only. Done. https://codereview.chromium.org/1141163002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:253: // Set up an user that doesn't have device ID. On 2015/05/28 00:18:57, achuithb wrote: > // Set up a user that doesn't have a device ID. Done. https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... File google_apis/gaia/fake_gaia.cc (right): https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.cc:140: auto maybe_update_field = On 2015/05/28 00:18:57, achuithb wrote: > The number of obscure C++ features used in this snippet of code is pretty > mind-boggling, but I guess this is ok. > > Could you add a comment like > // This lambda uses a pointer to data member to merge attributes. Done =). https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... File google_apis/gaia/fake_gaia.h (right): https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.h:35: typedef std::map<std::string, std::string> RefreshTokenToDeviceIdMap; On 2015/05/28 00:18:57, achuithb wrote: > Perhaps replace both typedefs with using? Done. https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.h:57: void Update(const MergeSessionParams& update); On 2015/05/28 00:18:57, achuithb wrote: > call args params for consistency? Done. https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.h:93: // Updates various params with non-empty values from |update|. On 2015/05/28 00:18:57, achuithb wrote: > |params| Done. https://codereview.chromium.org/1141163002/diff/60001/google_apis/gaia/fake_g... google_apis/gaia/fake_gaia.h:133: // Returns a device ID associated with a given refresh token. On 2015/05/28 00:18:57, achuithb wrote: > |refresh_token| Done.
lgtm. Thanks for taking care of the nits.
The CQ bit was checked by dzhioev@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/1141163002/#ps80001 (title: "Comments addressed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141163002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1e76fed394f8a90b049ce2c06ca06f677c899024 Cr-Commit-Position: refs/heads/master@{#331828} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
