Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2279863002:
Support server categories in NTPSnippetsService. (Closed)
|
Created:
4 years, 3 months ago by sfiera Modified:
4 years, 3 months ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org, Philipp Keck Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport server categories in NTPSnippetsService.
Reland of:
https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f
Not clear why tests failed; they passed for me and in the CQ.
Removed one synchronization bit that wasn't really part of the change.
BUG=633613
Committed: https://crrev.com/880bb766ee4985a8dd36be10098c5196c41f7509
Cr-Commit-Position: refs/heads/master@{#414681}
Patch Set 1 #Patch Set 2 : Reverse WaitForDBLoad change. #Patch Set 3 : rebase #
Total comments: 4
Messages
Total messages: 31 (17 generated)
The CQ bit was checked by sfiera@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 server categories in NTPSnippetsService. BUG=633613 ========== to ========== Support server categories in NTPSnippetsService. Reland of: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Not clear why tests failed; they passed for me and in the CQ. BUG=633613 ==========
sfiera@chromium.org changed reviewers: + treib@chromium.org
Not sure what protocol I should be following for relanding this, but I rebased and NTPSnippetsServiceTest.* still passes for me locally and the dry run is dry running fine and dry so far.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/08/25 17:49:56, sfiera wrote: > Not sure what protocol I should be following for relanding this, but I rebased > and NTPSnippetsServiceTest.* still passes for me locally and the dry run is dry > running fine and dry so far. Hm. I suppose it's possible that something was flaky already and you just got unlucky, highly unlikely that it would just suddenly fail on multiple bots simultaneously though. Is it possible your change to WaitForDBLoad messed something up? That's the only thing I can see here that might be related.
The CQ bit was checked by sfiera@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 2016/08/25 19:34:03, Marc Treib wrote: > On 2016/08/25 17:49:56, sfiera wrote: > > Not sure what protocol I should be following for relanding this, but I rebased > > and NTPSnippetsServiceTest.* still passes for me locally and the dry run is > dry > > running fine and dry so far. > > Hm. I suppose it's possible that something was flaky already and you just got > unlucky, highly unlikely that it would just suddenly fail on multiple bots > simultaneously though. > > Is it possible your change to WaitForDBLoad messed something up? That's the only > thing I can see here that might be related. Well, I went ahead and reverted it as it's orthogonal to this change, but the relevant code path doesn't, I think, involve it. Do you know how to run trigger a run on the bot that failed? (https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/45385) It doesn't seem to be one of the regular bots, and I don't know how I can avoid a failure I can't reproduce.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/25 20:16:15, sfiera wrote: > On 2016/08/25 19:34:03, Marc Treib wrote: > > On 2016/08/25 17:49:56, sfiera wrote: > > > Not sure what protocol I should be following for relanding this, but I > rebased > > > and NTPSnippetsServiceTest.* still passes for me locally and the dry run is > > dry > > > running fine and dry so far. > > > > Hm. I suppose it's possible that something was flaky already and you just got > > unlucky, highly unlikely that it would just suddenly fail on multiple bots > > simultaneously though. > > > > Is it possible your change to WaitForDBLoad messed something up? That's the > only > > thing I can see here that might be related. > > Well, I went ahead and reverted it as it's orthogonal to this change, but the > relevant code path doesn't, I think, involve it. > > Do you know how to run trigger a run on the bot that failed? > (https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/45385) > It doesn't seem to be one of the regular bots, and I don't know how I can avoid > a failure I can't reproduce. I think the waterfall is just set up a bit differently from the try bots here, it has separate Builder vs Test runner bots. The regular linux_chromium_dbg|rel_ng bots should cover the same thing.
On 2016/08/26 08:20:02, Marc Treib wrote: > On 2016/08/25 20:16:15, sfiera wrote: > > On 2016/08/25 19:34:03, Marc Treib wrote: > > > On 2016/08/25 17:49:56, sfiera wrote: > > > > Not sure what protocol I should be following for relanding this, but I > > rebased > > > > and NTPSnippetsServiceTest.* still passes for me locally and the dry run > is > > > dry > > > > running fine and dry so far. > > > > > > Hm. I suppose it's possible that something was flaky already and you just > got > > > unlucky, highly unlikely that it would just suddenly fail on multiple bots > > > simultaneously though. > > > > > > Is it possible your change to WaitForDBLoad messed something up? That's the > > only > > > thing I can see here that might be related. > > > > Well, I went ahead and reverted it as it's orthogonal to this change, but the > > relevant code path doesn't, I think, involve it. > > > > Do you know how to run trigger a run on the bot that failed? > > > (https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/45385) > > It doesn't seem to be one of the regular bots, and I don't know how I can > avoid > > a failure I can't reproduce. > > I think the waterfall is just set up a bit differently from the try bots here, > it has separate Builder vs Test runner bots. The regular > linux_chromium_dbg|rel_ng bots should cover the same thing. Ah, and LGTM - I'd just go ahead and re-land it, and watch the waterfall - if things start failing again, you could ping the sheriffs to let them know what's going on.
Description was changed from ========== Support server categories in NTPSnippetsService. Reland of: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Not clear why tests failed; they passed for me and in the CQ. BUG=633613 ========== to ========== Support server categories in NTPSnippetsService. Reland of: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Not clear why tests failed; they passed for me and in the CQ. Removed one synchronization bit that wasn't really part of the change. BUG=633613 ==========
The CQ bit was checked by sfiera@chromium.org
The CQ bit was unchecked by sfiera@chromium.org
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2279863002/#ps40001 (title: "rebase")
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.
Description was changed from ========== Support server categories in NTPSnippetsService. Reland of: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Not clear why tests failed; they passed for me and in the CQ. Removed one synchronization bit that wasn't really part of the change. BUG=633613 ========== to ========== Support server categories in NTPSnippetsService. Reland of: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Not clear why tests failed; they passed for me and in the CQ. Removed one synchronization bit that wasn't really part of the change. BUG=633613 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Support server categories in NTPSnippetsService. Reland of: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Not clear why tests failed; they passed for me and in the CQ. Removed one synchronization bit that wasn't really part of the change. BUG=633613 ========== to ========== Support server categories in NTPSnippetsService. Reland of: https://crrev.com/330958ddcdcd0084d724571eddaa66f9c364115f Not clear why tests failed; they passed for me and in the CQ. Removed one synchronization bit that wasn't really part of the change. BUG=633613 Committed: https://crrev.com/880bb766ee4985a8dd36be10098c5196c41f7509 Cr-Commit-Position: refs/heads/master@{#414681} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/880bb766ee4985a8dd36be10098c5196c41f7509 Cr-Commit-Position: refs/heads/master@{#414681}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2283743002/ by johnme@chromium.org. The reason for reverting is: I'm afraid this failed again on https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/273....
Message was sent while issue was closed.
https://codereview.chromium.org/2279863002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2279863002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:254: EXPECT_CALL(*observer_, OnCategoryStatusChanged(_, _, _)) If the DB loads before WaitForDBLoad is called, this EXPECT_CALL will be attached anyway and I don't know what happens when the next OnCategoryStatusChanged is executed. How about: if (!service->ready()) return; EXPECT_CALL() if (service->ready()) { Mock::ClearBla(observer); return; } run_loop_.Run()
Message was sent while issue was closed.
https://codereview.chromium.org/2279863002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2279863002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:254: EXPECT_CALL(*observer_, OnCategoryStatusChanged(_, _, _)) On 2016/08/26 12:03:20, Philipp Keck wrote: > If the DB loads before WaitForDBLoad is called, this EXPECT_CALL will be > attached anyway and I don't know what happens when the next > OnCategoryStatusChanged is executed. > > How about: > if (!service->ready()) return; > EXPECT_CALL() > if (service->ready()) { Mock::ClearBla(observer); return; } > run_loop_.Run() I guess the first line should read "if (service->ready() ..", i.e. no "!"? Anyway, Chris had a fix for this, but reverted it because it was the only thing that looked possibly related to the failing tests...
Message was sent while issue was closed.
https://codereview.chromium.org/2279863002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2279863002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:373: base::MakeUnique<NTPSnippetsDatabase>(database_dir_.path(), If you replace this line with: base::MakeUnique<NTPSnippetsDatabase>(database_dir_.path().DirName().Append("testsdf"), you get similar errors as the bot -- seemingly random expects fail. So a possibly cause could be that base::ScopedTempDir doesn't work as expected on that bot (or disk is full or sth like that).
Message was sent while issue was closed.
https://codereview.chromium.org/2279863002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2279863002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:373: base::MakeUnique<NTPSnippetsDatabase>(database_dir_.path(), On 2016/08/26 12:17:48, Philipp Keck wrote: > If you replace this line with: > base::MakeUnique<NTPSnippetsDatabase>(database_dir_.path().DirName().Append("testsdf"), > you get similar errors as the bot -- seemingly random expects fail. So a > possibly cause could be that base::ScopedTempDir doesn't work as expected on > that bot (or disk is full or sth like that). Huh. I guess we can conclude that the underlying problem is that these tests use a real DB that actually writes to disk (because I was too lazy to implement a fake DB - my bad). That causes other issues, e.g. it's the reason the random "base::RunLoop().RunUntilIdle();" in RecreateSnippetsService is necessary. So now we have a good reason to change that...
Message was sent while issue was closed.
The test needs a rewrite, in multiple ways. I guess that's what I'm doing today. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
