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

[ui] Add tests for recently visited assets #20321

Merged
merged 4 commits into from
Mar 7, 2024

Conversation

clairelin135
Copy link
Contributor

@clairelin135 clairelin135 commented Mar 7, 2024

Adds tests to preserve logic in #20266 that stores the N most recently-viewed assets:

  • Assert that when an asset is viewed, its asset key is stored in local storage
  • Assert that if a user tries to view a nonexistent asset, nothing is added to local storage

Let me know if more detailed tests are needed, i.e. tests asserting LRU cache behavior.

@clairelin135
Copy link
Contributor Author

clairelin135 commented Mar 7, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @clairelin135 and the rest of your teammates on Graphite Graphite

Copy link

github-actions bot commented Mar 7, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-g4ve158b7-elementl.vercel.app
https://03-06-claire-add-tests-recently-visited.core-storybook.dagster-docs.io

Built with commit 5718898.
This pull request is being automatically deployed with vercel-action

@clairelin135 clairelin135 force-pushed the 03-04-claire/recently-visited-history branch from f64b3fc to 0e3f279 Compare March 7, 2024 00:51
@clairelin135 clairelin135 force-pushed the 03-06-claire/add-tests-recently-visited branch from 4be4ae9 to 402da36 Compare March 7, 2024 00:51
@clairelin135 clairelin135 force-pushed the 03-04-claire/recently-visited-history branch from 0e3f279 to cf105b2 Compare March 7, 2024 01:24
@clairelin135 clairelin135 force-pushed the 03-06-claire/add-tests-recently-visited branch from 402da36 to 8513e6c Compare March 7, 2024 01:25
@clairelin135 clairelin135 marked this pull request as ready for review March 7, 2024 18:40
@clairelin135 clairelin135 requested a review from salazarm March 7, 2024 18:40
await waitFor(() => {
expect(screen.queryByText('Loading assets…')).toBeNull();
});
// Second render displays the page in a loaded state
Copy link
Contributor

Choose a reason for hiding this comment

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

expect(fetchRecentlyVisitedAssetsFromLocalStorage()).toEqual([]); to confirm it doesn't save it until after the loading finishes

@clairelin135 clairelin135 force-pushed the 03-04-claire/recently-visited-history branch from cf105b2 to bfc4326 Compare March 7, 2024 18:59
Base automatically changed from 03-04-claire/recently-visited-history to master March 7, 2024 19:11
@clairelin135 clairelin135 force-pushed the 03-06-claire/add-tests-recently-visited branch from 8513e6c to bc3d7f2 Compare March 7, 2024 19:12
@clairelin135 clairelin135 merged commit d3fe167 into master Mar 7, 2024
2 checks passed
@clairelin135 clairelin135 deleted the 03-06-claire/add-tests-recently-visited branch March 7, 2024 19:49
assetKey,
},
data: {
assetOrError: {
Copy link
Contributor

Choose a reason for hiding this comment

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

buildAssetNotFoundError for next time

PedramNavid pushed a commit that referenced this pull request Mar 28, 2024
Adds tests to preserve logic in
#20266 that stores the N most
recently-viewed assets:
- Assert that when an asset is viewed, its asset key is stored in local
storage
- Assert that if a user tries to view a nonexistent asset, nothing is
added to local storage

Let me know if more detailed tests are needed, i.e. tests asserting LRU
cache behavior.
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.

2 participants