Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2291733002:
Assert section ordering in NewTabPageAdapterTest (Closed)
|
Created:
4 years, 3 months ago by Philipp Keck Modified:
4 years, 3 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, Marc Treib Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAssert section ordering in NewTabPageAdapterTest
Introduce data class SectionDescriptor and helper class ItemsMatcher,
which checks expectations against the list of NewTabPageItems from the
NewTabPageAdapter. Adjust the assertItemsFor() method and its helper
methods to use the ItemsMatcher. Create new assertMatches() for
checking single sections.
BUG=None
Committed: https://crrev.com/f3afa6ede6bdb252657890c5498aa703853885b6
Cr-Commit-Position: refs/heads/master@{#415294}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Michael's comments #Messages
Total messages: 19 (12 generated)
pke@google.com changed reviewers: + bauerb@chromium.org, dgn@chromium.org, mvanouwerkerk@chromium.org, peconn@chromium.org
There was some discussion on previous CLs about the assertItemsFor() method. This CL implements some changes to the way the assertion works (note though that the test bodies themselves don't change). It doesn't check the total number of items, but also their sequence. So far, the following were equivalent, even though they shouldn't be: assertItemsFor(section(3), section(4)); assertItemsFor(section(4), section(3)); assertItemsFor(section(4), sectionWithMoreButton(2)); Checking the ordering could prove useful once we change it more dynamically. On the other hand, I don't know if doing this many checks would lead to less maintainable tests. Also, I'm not happy about some of the names (ItemsMatcher, for example), and there was some previous discussion around "assertItemsFor". I only implemented this CL because of an entry on my todo list; as something worth looking into. Let me know what you think, or after Wednesday (when my internship ends) just close or adjust or merge the CL as desired.
The CQ bit was checked by mvanouwerkerk@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...
lgtm with nits This test is a bit fiddly and will still need updates in multiple places when we add an item like the footer, but this helps, thanks! https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:54: public SectionDescriptor(boolean moreButton, boolean statusCard, int numSuggestions) { nit: blank line above this one. Ditto for the edits below. https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:59: assert numSuggestions == 0; Such asserts are generally no-ops, but we're already using Assert in this file, so why not use that? https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:72: private int mCurrentIndex = 0; nit: no need to assign 0, it's the default initialization. https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:89: public void expect(SectionDescriptor section) { nit: "descriptor" seems more distinct as an argument name https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:117: private void assertMatches(SectionDescriptor section, ItemGroup itemGroup) { nit: here also "descriptor" seems like a more distinct name for the first argument, especially as the second argument is expected to be the real section.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:54: public SectionDescriptor(boolean moreButton, boolean statusCard, int numSuggestions) { On 2016/08/30 10:35:35, Michael van Ouwerkerk wrote: > nit: blank line above this one. Ditto for the edits below. Done. https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:59: assert numSuggestions == 0; On 2016/08/30 10:35:35, Michael van Ouwerkerk wrote: > Such asserts are generally no-ops, but we're already using Assert in this file, > so why not use that? Done. https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:72: private int mCurrentIndex = 0; On 2016/08/30 10:35:35, Michael van Ouwerkerk wrote: > nit: no need to assign 0, it's the default initialization. Done. https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:89: public void expect(SectionDescriptor section) { On 2016/08/30 10:35:35, Michael van Ouwerkerk wrote: > nit: "descriptor" seems more distinct as an argument name Done. https://codereview.chromium.org/2291733002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:117: private void assertMatches(SectionDescriptor section, ItemGroup itemGroup) { On 2016/08/30 10:35:35, Michael van Ouwerkerk wrote: > nit: here also "descriptor" seems like a more distinct name for the first > argument, especially as the second argument is expected to be the real section. Done.
The CQ bit was checked by pke@google.com 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...
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 pke@google.com
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/2291733002/#ps20001 (title: "Michael's comments")
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Assert section ordering in NewTabPageAdapterTest Introduce data class SectionDescriptor and helper class ItemsMatcher, which checks expectations against the list of NewTabPageItems from the NewTabPageAdapter. Adjust the assertItemsFor() method and its helper methods to use the ItemsMatcher. Create new assertMatches() for checking single sections. BUG=None ========== to ========== Assert section ordering in NewTabPageAdapterTest Introduce data class SectionDescriptor and helper class ItemsMatcher, which checks expectations against the list of NewTabPageItems from the NewTabPageAdapter. Adjust the assertItemsFor() method and its helper methods to use the ItemsMatcher. Create new assertMatches() for checking single sections. BUG=None Committed: https://crrev.com/f3afa6ede6bdb252657890c5498aa703853885b6 Cr-Commit-Position: refs/heads/master@{#415294} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f3afa6ede6bdb252657890c5498aa703853885b6 Cr-Commit-Position: refs/heads/master@{#415294} |
