RC eligible because this is a test addition only patch.

Problem/Motivation

As noted in #2547833: Pager.inc -- add tests, clean it up, convert to a service, use it!, we are missing tests for the case when there are multiple pagers on a given page.

Proposed resolution

Add some :)

Remaining tasks

Review patch

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#4 2572533-4.patch8.17 KBmondrake
#4 interdiff_3-4.txt3.91 KBmondrake
#3 interdiff_2-3.txt1009 bytesmondrake
#3 2572533-3.patch6.84 KBmondrake
#2 2572533-2.patch6.84 KBmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
StatusFileSize
new 2572533-2.patch6.84 KB

Here's a proposal.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 2572533-3.patch6.84 KB
new interdiff_2-3.txt1009 bytes

Let's avoid some unnecessary drupal_gets

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new interdiff_3-4.txt3.91 KB
new 2572533-4.patch8.17 KB

Added more test cases, and introduced check for the actual page numbers being shown to users.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
Issue tags: +rc eligible

Tagging rc eligible

dawehner’s picture

dawehner
Primary language German
commented
Component: base system » database system

At least views itself should certainly have such tests already
Moving to the right component.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -rc eligible

The tests look good to me.
The patch still applies.
The test tests what the issue summary is needed.
There are different combinations of input queries tested.
It is a pitty that this is not a PHPUnit test. We could have created a provider function.
It all looks good to me. So for me it is RTBC.

This issue is needed for #2044435: Convert pager.inc to a service.

Good work mondrake!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2572533-4.patch, failed testing.

daffie’s picture

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

The testbot failures are not related to this patch. So back to RTBC.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Version: 8.1.x-dev » 8.2.x-dev

8.1.x test failure seems unrelated to me. Bumping to 8.2.x and launched tests on that branch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Nice ! Extra test coverage.

Committed and pushed a7955198f10b3041fc10d1284a383f2a73e7e0f7 to 8.2.x and e762c84 to 8.1.x. Thanks!

  • alexpott committed a795519 on 8.2.x 
    Issue #2572533 by mondrake: Add tests for multiple pagers on a given...

  • alexpott committed e762c84 on 8.1.x 
    Issue #2572533 by mondrake: Add tests for multiple pagers on a given...

Status: Fixed » Closed (fixed)

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