#1754216: Meta: Add one test that tests each plugin type intensive

Most of the logic in PagerPluginBase is already covered in PagerTest.php: function testPagerApi(). But the tests are written against the Full pager. So, the task here is to:

  1. write a minimum test pager type that extends PagerPluginBase,
    in views/tests/modules/views_test_data/lib/Drupal/views_test_data/Plugin/views
  2. add a new view based on this pager type,
    in views/tests/modules/views_test_config/test_views
  3. convert testPagerApi to use this new view, instead of 'test_pager_full'
  4. add test to cover function hasMoreRecords() in PagerPluginBase.
CommentFileSizeAuthor
#8 vdc-1754254-8.patch8.06 KBdawehner
#6 vdc-1754254-6.patch8.49 KBdawehner
#6 interdiff.txt7.12 KBdawehner
#4 1754254.patch4.15 KBdamiankloip

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs tests
herom’s picture

herom commented

issue summary updated.

damiankloip’s picture

damiankloip commented
Status: Active » Needs review
StatusFileSize
new 1754254.patch4.15 KB

We should actually be able to test this OK with PHPUnit. Using the mocking, we can get a mock for the abstract PagerPluginBase class and test all the methods.

Then we have a proper test for just testing this base functionality :) what do you think?

I have not got time to finish this right now, so if anyones else so desires, be my guest... I've left a todo in the test class.

herom’s picture

herom commented

@damiankloip I wrote the current summary based on the other issues in the Meta issue.
The PHPUnit looks fine too, I guess. but in that case, the testPagerApi function in PagerTest should be removed, because this is just duplicating that test.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: -Needs tests
StatusFileSize
new interdiff.txt7.12 KB
new vdc-1754254-6.patch8.49 KB

Great test coverage so far!

Wow, mocking the dbtng objects isn't straighforward.
@herom
I don't see where PagerTest::testPagerApi directly tests these functions (note: the call through the executable is a worthwile test).

damiankloip’s picture

damiankloip commented 
  1. +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/pager/PagerPluginBaseTest.php
@@ -0,0 +1,266 @@
+   * @see \Drupal\views\Plugin\views\pager\PagerPluginBase::getItemsPerPage()

    Good docs , but this seems a bit overkill to me. We have an @see on the class to the PagerPluginBase class, then the 'Tests the .... method' part, then an @see too! 

  2. +++ b/core/phpunit.xml.dist
@@ -1,6 +1,6 @@
-<phpunit bootstrap="tests/bootstrap.php" colors="true" strict="true">
+<phpunit bootstrap="tests/bootstrap.php" colors="true">

    Did that change get reverted already?

Otherwise, nice additions!

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new vdc-1754254-8.patch8.06 KB

Good docs , but this seems a bit overkill to me. We have an @see on the class to the PagerPluginBase class, then the 'Tests the .... method' part, then an @see too!

The reason why I do like this is because I can just directly jump to the code.

Removed the phpunit "hack".

damiankloip’s picture

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

This coverage looks great now.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

update issue summary