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

Update pelias-parser #1420

Merged
merged 1 commit into from
Jun 2, 2020
Merged

Update pelias-parser #1420

merged 1 commit into from
Jun 2, 2020

Conversation

Joxit
Copy link
Member

@Joxit Joxit commented Apr 17, 2020

Backgroud

Our parser rocks ! We stick the version of the perser in c0749a0.

But we did some good improvement since v1.38.0 such as

So?...

The parsing is evolving every day, and results can change and may break CI...

We definitely need to update the parser, but there are a few more questions...

  • Should we reduce parser specific tests from the API ? Such as autocomplete-like test which can change at every releases....
  • Should we duplicate tests in API and parser to monitor changes like Calle Principal Bar ?
  • Should we stay like this and do manual updates ? 🤷‍♂️

I removed 2 tests due to parser response order

  • Calle Principal Bar
  • Calle Principal Barc

@missinglink
Copy link
Member

missinglink commented Apr 17, 2020

Yeah good question.

Regarding package.json:

When pelias/parser was such a new project, we decided to pin to a specific version number for the 'big release', just so we could go through the extensive testing and be 100% sure we were releasing what we tested.

Now that the pelias/parser repo has matured we may choose to relax the version matching to a semver range (such as 1.x) since the public API is likely to stay fairly stable anyway (we can always major bump if required), this will mean feature: and fix: commits would be pulled in automatically.

Regarding the tests:

The test/unit/sanitizer/_text_pelias_parser.js file is covering a mixture of the responses from pelias/parser with the code used to interpret the solutions, so I think it's hard to completely avoid testing pelias/parser unless we mock/stub it out in tests?

In general, I think it's better to add test cases directly to the pelias/parser repo where possible, rather than adding them to pelias/api.

Somehow its nice to have the tests failing, so we can see what changed and decide if we're ready to accept the new functionality yet or not (in this repo).

Although the flip-side of that is that we might end up maintaining tests in two places and then have to update them in both places for every change, be it positive or negative.

I'm happy to continue as-is with the tests until they become a maintenance pain.
For the versioning, we can begin to relax to a semver version?

@orangejulius thoughts?

@orangejulius
Copy link
Member

orangejulius commented Apr 17, 2020

Despite it being a departure from our normal way of handling Pelias dependencies, I think realistically upgrading the parser version in the API has to be a manual operation.

The parser is not a simple library that we can comprehensively cover with unit tests, unlike something like iso-639-3, to pick another API dependency. Ultimately, confidence in the parser comes from running the acceptance tests on a full planet or other large build.

While we run acceptance tests against branches of the parser during development, the most straightforward place to decide "this code passes the acceptance tests for a given data build" seems to me to be the API. We could try to make that decision when merging into the parser, but that feels very indirect and we'd always have to do some level of testing at the API level anyway (not to mention fixing API unit tests as Joxit mentioned).

So my vote is we continue to periodically open PRs like this one, and then we can essentially do our normal API workflow: evaluate whether the changes in the PR are good or bad for the acceptance tests and go from there.

@missinglink
Copy link
Member

missinglink commented May 7, 2020

Can we please combine this PR #1429
closes #1429

@orangejulius last I recall you were testing this, is it good to merge?

@orangejulius orangejulius force-pushed the joxit/feat/pelias-parser branch from 67833f8 to 8c0914e Compare May 7, 2020 15:58
@msschl
Copy link

msschl commented May 7, 2020

I'm closing #1429 since it's a duplicate.

v1.45.1 is the latest version of pelias/parser

@orangejulius orangejulius force-pushed the joxit/feat/pelias-parser branch from 8c0914e to 2eb0205 Compare May 7, 2020 17:59
@orangejulius
Copy link
Member

I ran all our test suites on this PR comparing it to master with the same data. Overall there were only minor differences. A little jitter added here, a little taken away here.

Here are some screenshots of some diffs of the autocomplete test output:
Screenshot_2020-05-07_16-16-11
Screenshot_2020-05-07_16-15-49
Screenshot_2020-05-07_16-15-33

I think this PR in particular is good to go.

For later versions of the parser, there are issues with the Pelias unit tests. @missinglink if you can resolve those in a new PR, I'll test that one too.

@missinglink missinglink merged commit e379a5f into master Jun 2, 2020
@missinglink missinglink deleted the joxit/feat/pelias-parser branch June 2, 2020 11:10
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.

4 participants