Skip to content

Fix geo point with tests #154

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

Merged
merged 7 commits into from
Feb 20, 2023
Merged

Conversation

oplik0
Copy link
Contributor

@oplik0 oplik0 commented Feb 18, 2023

Pull Request

Related issue

Fixes #143

What does this PR do?

  • this is a corrected version of Fix _geo point not being correctly parsed #147 - the issue with tests was a Jest issue that appears to have been fixed in v28.
  • While experimenting with this, I also updated the firebase packages - decided to leave them at newer versions instead of rolling these changes back
  • Made some small changes to tests to align them with output format of adaptFields
  • Re-added logging geopoint information to adaptFields to align with tests
  • Added eslint-import-resolver-typescript, because without it the eslint import plugin doesn't work with exports in package.json which marked all firebase-admin imports as lint errors.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

@bidoubiwa
Copy link
Contributor

Hey @oplik0

Thanks a lot for this contribution 🙏

@oplik0
Copy link
Contributor Author

oplik0 commented Feb 20, 2023

It seems the snapshot test now need a flag added for CI:

New snapshot was not written. The update flag must be explicitly passed to write a new snapshot.
This is likely because this test is run in a continuous integration (CI) environment in which snapshots are not written by default.

Tests pass locally for me and in a fresh Codespaces environment:
working in Codespaces

@oplik0
Copy link
Contributor Author

oplik0 commented Feb 20, 2023

Well, turns out there are two simple solutions:

  • the recommended way is apparently to remove the snapshots from .gitignore and add them to the repository
  • or if updating the snapshots is preferred --updateSnapshot flag can just be added to arguments in test:coverage in package.json, overriding the default CI behavior.

Reference: https://jestjs.io/docs/snapshot-testing#are-snapshots-written-automatically-on-continuous-integration-ci-systems

For now I decided to follow Jest recommendations and commit the snapshot files. Please let me know if you prefer to leave them ignored.

The tests all pass now: https://github.com/oplik0/firestore-meilisearch/actions/runs/4223949285

@bidoubiwa
Copy link
Contributor

For now I decided to follow Jest recommendations and commit the snapshot files. Please let me know if you prefer to leave them ignored.

Good for me :)

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Very nice 🔥

@bidoubiwa bidoubiwa added the enhancement New feature or request label Feb 20, 2023
@bidoubiwa
Copy link
Contributor

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Feb 20, 2023

@meili-bors meili-bors bot merged commit c605539 into meilisearch:fix_geo_point Feb 20, 2023
@bidoubiwa bidoubiwa added skip-changelog The PR will not appear in the release changelogs and removed skip-changelog The PR will not appear in the release changelogs enhancement New feature or request labels Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog The PR will not appear in the release changelogs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants