Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522593010:
Remove warnings in org.chromium.chrome.browser.ntp.* code and JUnit tests. (Closed)
|
Created:
4 years ago by Bernhard Bauer Modified:
4 years ago CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove warnings in org.chromium.chrome.browser.ntp.* code and JUnit tests.
BUG=616090
Committed: https://crrev.com/c8ecd6531500968e2babc28de6604e1887060358
Cr-Commit-Position: refs/heads/master@{#434491}
Patch Set 1 #Patch Set 2 : x #Patch Set 3 : x #Patch Set 4 : x #
Total comments: 18
Patch Set 5 : review #Patch Set 6 : fixed tests #
Total comments: 14
Patch Set 7 : fix #Patch Set 8 : review #Messages
Total messages: 45 (29 generated)
The CQ bit was checked by bauerb@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 bauerb@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 bauerb@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...
bauerb@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Please review.
lgtm with nits https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:36: private boolean mHasMoreAction = false; It's false by default. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:37: private boolean mHasViewAllAction = false; It's false by default. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:39: private boolean mShowIfEmpty = false; It's false by default. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:92: SuggestionsCategoryInfo category = You could return without assigning a local: return Builder.build(); https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:94: .hasMoreAction(false) Shouldn't these be indented further?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dgn@chromium.org changed reviewers: + dgn@chromium.org
https://codereview.chromium.org/2522593010/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:142: assert mIsAssetDownload; nit: isDownload() is redundant with that one, since we check it before setting mIsAssertDownload to true https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:38: private boolean mHasReloadAction = true; why not use false as default value here? That would be more predictable I think, as everything else has default values. Or specify in the documentation what the default configuration corresponds to. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:39: private boolean mShowIfEmpty = false; It looks like you wrote some tests while mShowIfEmpty was true by default, then changed it and went to write the rest of the tests. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:619: .showIfEmpty(false) false is the default, no need to set it again, esp. since it's irrelevant for the test. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:181: .build(); showIfEmpty was previously true, now false.
Also, there is another warning in the tests: warning: [deprecation] MediumTest in android.test.suitebuilder.annotation has been deprecated import android.test.suitebuilder.annotation.MediumTest; (see compilation logs: https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... Sadly they stop at 100 warnings so we can't see anything else.) Should we address it now or wait until it is done at the scale of the whole codebase at once?
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Thanks for the review! https://codereview.chromium.org/2522593010/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:142: assert mIsAssetDownload; On 2016/11/23 14:12:51, dgn wrote: > nit: isDownload() is redundant with that one, since we check it before setting > mIsAssertDownload to true That's true, but it doesn't hurt to assert that invariant here as well, right? I've updated the comment. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:36: private boolean mHasMoreAction = false; On 2016/11/23 13:43:56, Michael van Ouwerkerk wrote: > It's false by default. Done. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:37: private boolean mHasViewAllAction = false; On 2016/11/23 13:43:56, Michael van Ouwerkerk wrote: > It's false by default. Done. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:39: private boolean mShowIfEmpty = false; On 2016/11/23 14:12:51, dgn wrote: > It looks like you wrote some tests while mShowIfEmpty was true by default, then > changed it and went to write the rest of the tests. You're right, I think I made |mHasReloadAction| the default-true by accident instead of this one. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:92: SuggestionsCategoryInfo category = On 2016/11/23 13:43:56, Michael van Ouwerkerk wrote: > You could return without assigning a local: return Builder.build(); Done. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:94: .hasMoreAction(false) On 2016/11/23 13:43:56, Michael van Ouwerkerk wrote: > Shouldn't these be indented further? This instance is gone now. There is similar code in SuggestionsSectionTest, where I used Eclipse auto-indenting, and to me personally this looks okay, but I can change it if you want me to. https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:619: .showIfEmpty(false) On 2016/11/23 14:12:51, dgn wrote: > false is the default, no need to set it again, esp. since it's irrelevant for > the test. Ok. I've removed all no-op calls, which means that for the other calls we can now remove the parameter. It looks pretty nice now, I think! https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2522593010/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:181: .build(); On 2016/11/23 14:12:51, dgn wrote: > showIfEmpty was previously true, now false. Oops, fixed.
On 2016/11/23 14:30:18, dgn wrote: > Also, there is another warning in the tests: > > warning: [deprecation] MediumTest in android.test.suitebuilder.annotation has > been deprecated > import android.test.suitebuilder.annotation.MediumTest; > > (see compilation logs: > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > Sadly they stop at 100 warnings so we can't see anything else.) > > Should we address it now or wait until it is done at the scale of the whole > codebase at once? Yeah, there are a bunch of warnings in the instrumentation tests, which I haven't worked on yet at all. I think those showed up recently... maybe with an SDK roll or something? In that case it would make more sense to tackle all of those at once.
lgtm
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2522593010/#ps120001 (title: "review")
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
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...)
The CQ bit was checked by bauerb@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...
OK, so it turned out I had completely messed up the CategoryInfo logic in a couple of places. The JUnit tests now pass again (hooray for test coverage!), but would you mind sanity-checking?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
there are a bunch of places where we set reload to true while it's not needed. Not sure if it's worth removing. It's won't change the results anyway. There is only one where it seems to not have an effect while it should... bug or am I reading it wrong? https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:475: new CategoryInfoBuilder(category).withReloadAction().showIfEmpty().build()); Looks like something is broken here: category info says to show the reload action the initial state asserts for section with status card (no button) and progress. Shouldn't a button be shown here, since there are no suggestions? Also, the reload action is supposed to be irrelevant to that bit of test, it could be left to its default value, same below. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:533: .withReloadAction() withReloadAction is not useful here, view all means it will always be shown anyway. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:621: .withReloadAction() reload has no effect here too https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:634: new CategoryInfoBuilder(dynamicCategory1).withReloadAction().build()); reloadAction has no effect here too https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:997: new CategoryInfoBuilder(KnownCategories.ARTICLES).withReloadAction().build()); reload has no effect here too https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:1018: /** Registers the category with hasMoreButton=false and showIfEmpty=true */ update this to say that has only the reload action enabled? Changing it to have reload disabled by default will probably imply changing a few indices for empty state cases that would lose the action item, not sure it's worth bothering with that. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:211: // the suppression. probably needs rebasing as vitalii fixed this.
The CQ bit was checked by bauerb@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...
Thanks for double-checking! https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:475: new CategoryInfoBuilder(category).withReloadAction().showIfEmpty().build()); On 2016/11/24 16:19:53, dgn wrote: > Looks like something is broken here: > > category info says to show the reload action > the initial state asserts for section with status card (no button) and progress. > Shouldn't a button be shown here, since there are no suggestions? > > Also, the reload action is supposed to be irrelevant to that bit of test, it > could be left to its default value, same below. Indeed! As it turns out, the section descriptor was always expecting an action in an empty section (and most of the time the generated category infos had one by default). I've changed the descriptor interface to just mention "action" to avoid the impression that we're verifying the exact type of action. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:533: .withReloadAction() On 2016/11/24 16:19:52, dgn wrote: > withReloadAction is not useful here, view all means it will always be shown > anyway. Done. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:621: .withReloadAction() On 2016/11/24 16:19:53, dgn wrote: > reload has no effect here too Done. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:634: new CategoryInfoBuilder(dynamicCategory1).withReloadAction().build()); On 2016/11/24 16:19:52, dgn wrote: > reloadAction has no effect here too Done. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:997: new CategoryInfoBuilder(KnownCategories.ARTICLES).withReloadAction().build()); On 2016/11/24 16:19:52, dgn wrote: > reload has no effect here too Done. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:1018: /** Registers the category with hasMoreButton=false and showIfEmpty=true */ On 2016/11/24 16:19:52, dgn wrote: > update this to say that has only the reload action enabled? Changing it to have > reload disabled by default will probably imply changing a few indices for empty > state cases that would lose the action item, not sure it's worth bothering with > that. Done. https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2522593010/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:211: // the suppression. On 2016/11/24 16:19:53, dgn wrote: > probably needs rebasing as vitalii fixed this. Hooray! Done.
The CQ bit was checked by bauerb@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...
Thanks! lgtm
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 bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2522593010/#ps180001 (title: "review")
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": 180001, "attempt_start_ts": 1480080197046120,
"parent_rev": "bb6534a20c3d3de27860bb516034057c31d80ea1", "commit_rev":
"597dd8a16ee4dc742463eb1a9a1cef86b81092ab"}
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Remove warnings in org.chromium.chrome.browser.ntp.* code and JUnit tests. BUG=616090 ========== to ========== Remove warnings in org.chromium.chrome.browser.ntp.* code and JUnit tests. BUG=616090 Committed: https://crrev.com/c8ecd6531500968e2babc28de6604e1887060358 Cr-Commit-Position: refs/heads/master@{#434491} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c8ecd6531500968e2babc28de6604e1887060358 Cr-Commit-Position: refs/heads/master@{#434491} |
