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.

Sign up for GitHub

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

Jump to bottom

fix: test case fails for diffPlaces #1644

Closed
wasi-m opened this issue Dec 22, 2022 · 3 comments · Fixed by #1655
Closed

fix: test case fails for diffPlaces #1644

wasi-m opened this issue Dec 22, 2022 · 3 comments · Fixed by #1655
Labels
bug

Comments

@wasi-m
Copy link

wasi-m commented Dec 22, 2022

Describe the bug

The Test case fails for the diff Places

Steps to Reproduce

Clone dev branch, and run the tests

PELIAS_CONFIG=./test/test-pelias-config.json node --trace-uncaught test/unit/run.js | npx tap-dot

Expected behavior

Should check for grolmanstrasse

Environment (please complete the following information):

  • OS: Mac
  • Docker version: Docker version 20.10.21, build baeda1f
  • Docker Compose version: Docker Compose version v2.12.2

Pastebin/Screenshots

image
@wasi-m wasi-m added the bug label Dec 22, 2022
@wasi-m wasi-m mentioned this issue Dec 22, 2022
Closed
@michaelkirk
Copy link
Contributor

This test is failing for me too.

It seems like the underlying remove-accents package is implicated.

The test is depending on behavior from this change: tyxla/remove-accents#12 included first in release v0.4.2.

But that behavior was reverted in 0.4.4 released in November '22.
See tyxla/remove-accents#31

I don't really know what behavior is better. But if we want to just delegate that decision to remove-accents, then as well as updating this test, it might make sense to bump the minimum version of remove-accents to 0.4.4 to keep everything coherent.

@missinglink missinglink mentioned this issue Jun 23, 2023
Closed
@missinglink
Copy link
Member

Hi, thanks for tracking this down, I alerted the author in the linked issue above.

michaelkirk added a commit to michaelkirk-pelias/api that referenced this issue Jun 30, 2023
@michaelkirk
DO NOT MERGE: This is part of pelias#1644
d3e0b90 
It's unclear to me what direction the maintainers want to go with it,
but in the meanwhile, I want the tests to pass.
orangejulius added a commit that referenced this issue Jul 2, 2023
@orangejulius
fix(deps): Pin remove-accents to 0.4.2
b04d0d3 
A unit test has been failing because this dependency added normalization
to the German [Eszet](https://en.wikipedia.org/wiki/%C3%9F) character.

From a issue thread it sounds like we are going to encourage the
maintainers to back out that change, but until then pin to a specific
known-good version so that our unit tests pass :)

Fixes #1644
Connects tyxla/remove-accents#12
@orangejulius orangejulius mentioned this issue Jul 2, 2023
Merged
@orangejulius
Copy link
Member

I just merged #1655 to pin to a version of remove-accents that operates how our unit tests expect and we believe to be correct. Sounds like we can undo that pin in the near future though, as remove-accents may revert the change that broke our tests. Until then tests are passing again for everyone :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Pin remove-accents to 0.4.2 pelias/api
4 participants
@orangejulius @michaelkirk @missinglink @wasi-m