Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931453002:
Re-enable reading list tests (Closed)
|
Created:
3 years, 6 months ago by huangml1 Modified:
3 years, 6 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, Eugene But (OOO till 7-30), baxley+watch_chromium.org, ios-reviews+web_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, stkhapugin, huangml+watch_chromium.org, liaoyuke+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-enable reading list tests
The tests are disabled for landing the new server. Here is the fix for them.
BUG=724555
Review-Url: https://codereview.chromium.org/2931453002
Cr-Commit-Position: refs/heads/master@{#478019}
Committed: https://chromium.googlesource.com/chromium/src/+/c72b03e973cdca53fabc5b5f2e3776e4ae478e7a
Patch Set 1 #
Total comments: 12
Patch Set 2 : Remove global #Patch Set 3 : capital method name #
Total comments: 4
Patch Set 4 : nit #
Total comments: 2
Messages
Total messages: 18 (8 generated)
Description was changed from ========== Re-enable reading list tests BUG= ========== to ========== Re-enable reading list tests The tests are disabled for landing the new server. Here is the fix for them. BUG=724555 ==========
huangml@chromium.org changed reviewers: + baxley@chromium.org
Re-enabling a few reading list tests which were disabled for landing the new server. PTAL, thanks.
Just a couple of questions, thanks! https://codereview.chromium.org/2931453002/diff/1/ios/chrome/browser/ui/readi... File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right): https://codereview.chromium.org/2931453002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:377: kDistillableFullURL = web::test::HttpServer::MakeUrl(kDistillableURL); Can we just replace kDistillableRullURL on line 340 with the right side of this, and then delete the global? https://codereview.chromium.org/2931453002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:487: server.setSuspend(NO); What if the assert above fails? will the server be unsuspended for the next test (or teardown)? https://codereview.chromium.org/2931453002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:524: server.setSuspend(NO); Same as last question, will this always get set properly, if something fails between the two calls to setSuspend? https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... File ios/web/public/test/http_server/http_server.h (right): https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... ios/web/public/test/http_server/http_server.h:84: void setSuspend(BOOL suspended); would it be more consistent if this is "bool"? what do you think? https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... ios/web/public/test/http_server/http_server.h:141: // The status that if the server hangs. "Status tracking if the server is hung." https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... ios/web/public/test/http_server/http_server.h:142: BOOL isSuspended; bool?
https://codereview.chromium.org/2931453002/diff/1/ios/chrome/browser/ui/readi... File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right): https://codereview.chromium.org/2931453002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:377: kDistillableFullURL = web::test::HttpServer::MakeUrl(kDistillableURL); On 2017/06/06 18:14:16, baxley wrote: > Can we just replace kDistillableRullURL on line 340 with the right side of this, > and then delete the global? Yes we can! Sorry that I mixed it up with previous approach when server is killed the makeUrl does not work at line 340. https://codereview.chromium.org/2931453002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:487: server.setSuspend(NO); On 2017/06/06 18:14:17, baxley wrote: > What if the assert above fails? will the server be unsuspended for the next test > (or teardown)? You're right. I missed the condition. Now I stop suspending the server when tearing down each test to make the following one in a clean state. https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... File ios/web/public/test/http_server/http_server.h (right): https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... ios/web/public/test/http_server/http_server.h:84: void setSuspend(BOOL suspended); On 2017/06/06 18:14:17, baxley wrote: > would it be more consistent if this is "bool"? what do you think? I feel that setSuspend is more like an action than returning a state. Do you think we should make it consistent with the bool IsRunning Style? https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... ios/web/public/test/http_server/http_server.h:141: // The status that if the server hangs. On 2017/06/06 18:14:17, baxley wrote: > "Status tracking if the server is hung." Done. https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... ios/web/public/test/http_server/http_server.h:142: BOOL isSuspended; On 2017/06/06 18:14:17, baxley wrote: > bool? Done.
One style nit, and question about side-effects of turning off stopped server. Once nit is addressed, and if there are no side-effects to the line I asked about, LGTM. You'll still need owners for the *egtest.mm https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... File ios/web/public/test/http_server/http_server.h (right): https://codereview.chromium.org/2931453002/diff/1/ios/web/public/test/http_se... ios/web/public/test/http_server/http_server.h:84: void setSuspend(BOOL suspended); On 2017/06/07 18:10:54, huangml1 wrote: > On 2017/06/06 18:14:17, baxley wrote: > > would it be more consistent if this is "bool"? what do you think? > > I feel that setSuspend is more like an action than returning a state. Do you > think we should make it consistent with the bool IsRunning Style? I meant more that the rest of this is C++. If the callers are Objective-C then I guess it's okay, and maybe better? Both should be fine. https://codereview.chromium.org/2931453002/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right): https://codereview.chromium.org/2931453002/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:342: .GetContent())] nit: what do you think about creating a variable for the URL, and possibly add "using chrome_test_util::OmniboxText" to make this fit on one line? Or anything else to make this line shorter? https://codereview.chromium.org/2931453002/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:385: server.SetSuspend(NO); are there any side-effects to calling this after tests that don't suspend the server?
huangml@chromium.org changed reviewers: + gambard@chromium.org
+gambard for owner review, thanks! https://codereview.chromium.org/2931453002/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right): https://codereview.chromium.org/2931453002/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:342: .GetContent())] On 2017/06/07 18:21:06, baxley wrote: > nit: what do you think about creating a variable for the URL, and possibly add > "using chrome_test_util::OmniboxText" to make this fit on one line? > > Or anything else to make this line shorter? Done. https://codereview.chromium.org/2931453002/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:385: server.SetSuspend(NO); On 2017/06/07 18:21:06, baxley wrote: > are there any side-effects to calling this after tests that don't suspend the > server? No. The method just sets the variable to "NO" and cause no effect on the responseProvider.
lgtm
The CQ bit was checked by huangml@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org Link to the patchset: https://codereview.chromium.org/2931453002/#ps60001 (title: "nit")
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": 60001, "attempt_start_ts": 1496943542145100,
"parent_rev": "ea0116aad7867695cb2e438d3546f148ea271858", "commit_rev":
"c72b03e973cdca53fabc5b5f2e3776e4ae478e7a"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable reading list tests The tests are disabled for landing the new server. Here is the fix for them. BUG=724555 ========== to ========== Re-enable reading list tests The tests are disabled for landing the new server. Here is the fix for them. BUG=724555 Review-Url: https://codereview.chromium.org/2931453002 Cr-Commit-Position: refs/heads/master@{#478019} Committed: https://chromium.googlesource.com/chromium/src/+/c72b03e973cdca53fabc5b5f2e37... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c72b03e973cdca53fabc5b5f2e37...
Message was sent while issue was closed.
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
Message was sent while issue was closed.
Driving by https://codereview.chromium.org/2931453002/diff/60001/ios/web/public/test/htt... File ios/web/public/test/http_server/http_server.h (right): https://codereview.chromium.org/2931453002/diff/60001/ios/web/public/test/htt... ios/web/public/test/http_server/http_server.h:142: bool isSuspended; s/isSuspended;/is_suspended_ = false; C++ Style and initialize, because POD types do not have default values in C++ https://codereview.chromium.org/2931453002/diff/60001/ios/web/public/test/htt... File ios/web/public/test/http_server/http_server.mm (right): https://codereview.chromium.org/2931453002/diff/60001/ios/web/public/test/htt... ios/web/public/test/http_server/http_server.mm:115: isSuspended = NO; = false;
Message was sent while issue was closed.
On 2017/06/12 02:25:44, Eugene But (OOO till June 11) wrote: > Driving by > > https://codereview.chromium.org/2931453002/diff/60001/ios/web/public/test/htt... > File ios/web/public/test/http_server/http_server.h (right): > > https://codereview.chromium.org/2931453002/diff/60001/ios/web/public/test/htt... > ios/web/public/test/http_server/http_server.h:142: bool isSuspended; > s/isSuspended;/is_suspended_ = false; > > C++ Style and initialize, because POD types do not have default values in C++ > > https://codereview.chromium.org/2931453002/diff/60001/ios/web/public/test/htt... > File ios/web/public/test/http_server/http_server.mm (right): > > https://codereview.chromium.org/2931453002/diff/60001/ios/web/public/test/htt... > ios/web/public/test/http_server/http_server.mm:115: isSuspended = NO; > = false; Thanks for the review! I'll upload another CL to fix. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
