Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(data): add DataStore query pagination unit tests #13129

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

david-mcafee
Copy link
Member

@david-mcafee david-mcafee commented Mar 15, 2024

Description of changes

Adding unit tests that reproduce #12049. See comments for which tests are failing.

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@david-mcafee david-mcafee requested review from a team as code owners March 15, 2024 21:56
@@ -65,6 +69,25 @@ describe('Indexed db storage test', () => {
);
});

afterEach(async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into a strange error when reproducing this issue. In the final test, where I create a single record with a page value greater than 0 (which should not return any records), I was received 10+ records that were not being returned by any of the tests. I attempted to clear / reset DataStore at a more fundamental level in this test, but decided to timebox the effort and add this step as a temporary cleanup solution so that results from previous tests did not bleed into the other tests.

Copy link
Contributor

@svidgen svidgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a crufty comment.

Also, could/should these tests by common adapter tests instead?

packages/datastore/__tests__/indexeddb.test.ts Outdated Show resolved Hide resolved
});

// Assertion fails - record is returned:
expect(secondPage.length).toEqual(0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test currently fails - once we've found a fix, this should pass.

});

// Assertion fails - record is returned:
expect(thirdPage.length).toEqual(0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test currently fails - once we've found a fix, this should pass.

@david-mcafee david-mcafee self-assigned this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants