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

[WIP] Start updating JS parser unit tests #69

Closed
wants to merge 2 commits into from

Conversation

ebrelsford
Copy link
Collaborator

The intention of this PR is to update JS parser unit tests to test smaller parts of the code and with simpler input MVT/MLT tiles in order to make updating the parser more straightforward. It's expected that tests on this branch will fail until the parser is updated.

@ebrelsford
Copy link
Collaborator Author

@springmeyer Here's a draft of the kinds of tests I'm thinking + the way I was planning on including mvt-fixtures, when you get a chance would be interested to hear your thoughts if any.

package.json Outdated Show resolved Hide resolved
import * as Path from "path";
import { toBeDeepCloseTo, toMatchCloseTo } from "jest-matcher-deep-close-to";
expect.extend({ toBeDeepCloseTo, toMatchCloseTo });

const tilesDir = "./data";

describe("CovtDecoder", () => {
it("should decode a simple valid tile", () => {
const tiles = getMvtFixtureTiles("017"); // Small valid MVT fixture
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also do mvtFixtures.get('017') (https://github.com/mapbox/mvt-fixtures/blob/main/API.md#get)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep! That's what I do below, I was using this method to try to stay aligned with how the other tile fetching happens in these tests (getting both the MVT and MLT at one time).

Copy link
Collaborator

@springmeyer springmeyer left a comment

Choose a reason for hiding this comment

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

Overall looking good / matches what I have had in mind. Great start.

One further idea: having the input mlt tiles generated dynamically. We could do this by having the JS tests call out to the java encoder CLI. Various pros/cons of doing this of course. Main cons are slowness and some potential brittleness. But the main benefits I see are: a) ensuring we are working with fresh/correct MLT tiles and not ones that are ever out of date or encoded with old java encoder bugs and b) then the JS tests will start failing (relevant once they are first passing) if the Java encoder changes in a way that is unexpected.

@springmeyer
Copy link
Collaborator

@ebrelsford another idea: considering having all test assertions be on JSON objects. We talked before about writing the tests to assert on each part of the decoded tile (which would be valuable for incrementally ensuring parts start working and for showing specifically what is failing to work). Still like this idea. But also, here is a quick alternative idea: to have the decoder create an entire GeoJSON in memory and then use a library like https://www.npmjs.com/package/json-diff to diff against an entire expected GeoJSON. This could accomplish the former goals with an additional benefit: as the tile fixtures grow increasingly complicated in terms of geometries we could then easily visualize by dumping the GeoJSON output to disk and tossing in a mapping program of choice. And of course the two approaches could be blended together.

@ebrelsford ebrelsford closed this Jun 18, 2024
@ebrelsford ebrelsford deleted the update-js-unit-tests-2 branch June 18, 2024 19:17
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

2 participants