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

chore(sync-service): Set display settings before querying initial data #232

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

alco
Copy link
Member

@alco alco commented Aug 1, 2024

Follow-up to #220.

Copy link
Contributor

@robacourt robacourt left a comment

Choose a reason for hiding this comment

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

Looks good!

You asked about tests - how about a test for query_in_readonly_txn specifically that actually hits the db in the shape_cache tests? Seems that could do with some tests anyway since it's normally mocked out. There's no module wide setup in the shape_cache tests so a new describe block which uses the TransactionCase should do it, right?

@robacourt
Copy link
Contributor

Looks good!

You asked about tests - how about a test for query_in_readonly_txn specifically that actually hits the db in the shape_cache tests? Seems that could do with some tests anyway since it's normally mocked out. There's no module wide setup in the shape_cache tests so a new describe block which uses the TransactionCase should do it, right?

Or another option is that query_in_readonly_txn is extracted to another module and tested separately.

@alco
Copy link
Member Author

alco commented Aug 1, 2024

@robacourt Thanks for the suggestions!

I hadn't noticed initially that ShapeCacheTest already had some tests running against a real database. I've added another one there to verify correct formatting for snapshot data.

Really wanted to avoid testing query_in_readonly_txn directly because that's an implementation detail. Ideally, we would test the response payload returned by ServerShapePlug. But testing ShapeCache is good enough for now.

@alco alco force-pushed the alco/initial-data-display-settings branch from 0790d76 to b07920b Compare August 1, 2024 20:41
@alco alco merged commit 5522305 into main Aug 1, 2024
13 checks passed
@alco alco deleted the alco/initial-data-display-settings branch August 1, 2024 20:43
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