Coverage increased (+0.1%) to 80.676% when pulling 7e99bed on justlevine:fix/connection-pagination into 16b159f on wp-graphql:develop.

First bug:

Fetching multiple comments in one query (or in one Codeception test instance), recycles the original ids instead of fetching new ones.

To replicate:

1. Create and approve 3 comments.
2. Run the following query

{
  comments(first:2) {
    pageInfo {
      startCursor
      endCursor
    }
    edges {
      cursor
      node {
        databaseId
      }
    }
  }
  after:comments(first:1, after:"YXJyYXljb25uZWN0aW9uOjM="){ # the second cursor.
    pageInfo {
      startCursor
      endCursor
    }
    edges {
      cursor
      node {
        databaseId
      }
    }
  }
}

All results should be shown correctly. The reason for this is that the first comments query overfetches by one.

3. Change the comments(first:2) to comments(first:1) and rerun the query.
   You should see after returns null. This is because the comment query overfetch only includes the first two.

Screenshots

• first:2
  
• first:1
  

@justlevine I see that this PR removed a lot of tests.

For example:

• tests/wpunit/CursorPaginationForPostsTest.php
• tests/wpunit/CursorPaginationForCategoriesTest.php

etc.

Were these removed because they're duplicate tests and have coverage elsewhere? or was there another reason for removing them?

Were these removed because they're duplicate tests and have coverage elsewhere? or was there another reason for removing them?

Mostly duplicates. What wasnt has been relocated to the {ConnectionType}QueriesTest/{ConnectionType}PaginationTest to keep a standard pattern and ensure tests arent duped/overlooked in the future.

Okay first pass is done:

Refactored AbstractConnectionResolver to better follow the Connection spec, while using smaller methods to allow easier creation of child classes. Of note.

• get_offset() is deprecated in favor of get_offset_for_cursor( string $cursor ), so we can support before + after together.
• get_offset_for_cursor() works with either a string|int offset, and is passed to get_array_index_for_offset( $offset, array $ids) to handle where in the array we slice.
• The big change is the lifecycle of when we're slicing/reversing our IDs:
  • get_ids() is no longer abstract and now runs get_ids_from_query() and apply_cursors_to_ids( array $ids ). The former gets the IDs from $this->query and sets them in the correct order; the latter handles the new overfetched slicing logic.
    For b/c get_ids_from_query() is not abstract. Instead an Exception is thrown if the class doesnt either implement that method or get_ids().
  • get_ids_for_nodes() slices (the overfetched) $this->ids to the actual connection query size.
• is_valid_offset() has been added to missing classes.

To Do

• Term/Taxonomy tests arent working because of old terms/taxes left over from previous tests. Need to find and remove them. 👋 help appreciated!
• Comments need cursor pagination support
• Add assert to confirm the order of first and last are the same.
• Add tests for Menu/MenuItem pagination.
• Cleanup code
• Double check no tests missing from Bug/#2409 menu item locations conflicts with taxonomy location #2411 commit

Those cests should not be failing 🤔

Turns out most tests don't clean up after themselves, and some use duplicate names for objects causing tests to pass that shouldnt.

Tried to catch/fix as many as I could. I think all thats left are WP_Post and WP_User objects.

Code Climate has analyzed commit c7d9c9b and detected 3 issues on this pull request.

Here's the issue category breakdown:

View more on Code Climate.

Work moved to #2457