This is a follow up to (in reverse order) #3059332: Mark kernel tests that perform no assertions as risky, #2553655: Convert ViewKernelTestBase to use KernelTestBaseTNG, and #1872876: Turn role permission assignments into configuration.

Problem/Motivation

Drupal\Tests\views\Kernel\Handler\FieldCounterTest has this code which will be removed by #3059332: Mark kernel tests that perform no assertions as risky:

  /**
   * @todo: Write tests for pager.
   */
  public function testPager() {
  }

Proposed resolution

Do the @todo.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3063179-11.patch3.23 KBshubham.prakash
#10 3063179-10.patch3.22 KBshubham.prakash
#4 3063179-4.patch3.22 KBLendude
#2 3063179-2.patch1.65 KBLendude
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
Status: Active » Needs review
FileSize
3063179-2.patch1.65 KB

Here we go....

daffie’s picture

daffie CreditAttribution: daffie commented

Hi @lendude, your tests looks good. But I am missing some testing for when the counter_start is used and for when the pager is set to not use the pager's count. Would you like to add those things to the test? If not then the patch is RTBC for me.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
FileSize
3063179-4.patch3.22 KB

@daffie thanks for the review! You are absolutely right, we should add coverage for the counter_start option too, so here we go.

Needed a reroll so no interdiff.

daffie’s picture

daffie CreditAttribution: daffie commented

@lendude: The added testing is RTBC for me. I have just one question left. In the counter handler that we now are testing for there is a piece of code and I am not sure if we need to add testing for it. What do you think lendude?

    if ($pager->usePager()) {
      $count += ($pager->getItemsPerPage() * $pager->getCurrentPage() + $pager->getOffset());
    }
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented

Isn't the entire new test method dedicated to that line? The idea was to add coverage for the counter field when using a pager, which is what we are doing here right?
Or am I overlooking something?

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

ok, then it is RTBC for me.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

Needs a re-roll after #3063180: REST views: View result counter shows {{counter}} placeholder when using "data_field" row plugin.

daffie’s picture

daffie CreditAttribution: daffie commented
Issue tags: +Needs reroll

Add the tag.

shubham.prakash’s picture

shubham.prakash CreditAttribution: shubham.prakash at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
3063179-10.patch3.22 KB

Here is the patch.

shubham.prakash’s picture

shubham.prakash CreditAttribution: shubham.prakash at OpenSense Labs commented
FileSize
3063179-11.patch3.23 KB

This patch should fix the issue.

daffie’s picture

daffie CreditAttribution: daffie commented
Issue tags: -Needs reroll

@Shubham Prakash: Could you post an interdiff.txt file? See: https://www.drupal.org/documentation/git/interdiff

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

@Shubham Prakash: Forget about the interdiff.txt file. Thank you for the reroll.

Back to RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed ece5db4 and pushed to 8.8.x. Thanks!

  • larowlan committed ece5db4 on 8.8.x 
    Issue #3063179 by Lendude, shubham.prakash, daffie: Add test coverage...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.