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

Create map-utils.unit.test.js #956

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

Create map-utils.unit.test.js #956

wants to merge 4 commits into from

Conversation

s-pic
Copy link
Collaborator

@s-pic s-pic commented Jan 20, 2022

that actually tests the calculation using a simple example. Goal is to detect regressions caused by updating the turf.js version

closes #769

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [-] I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

that actually tests the calculation using a simple example. Goal is to detect regressions caused by updating the turf.js version

closes #769
@s-pic s-pic requested a review from tordans January 20, 2022 19:53
@s-pic
Copy link
Collaborator Author

s-pic commented Jan 20, 2022

Acutally wanted to

  • simplify the function by not just returning a given default coordinate (can be simplified by just adding a default value at the call site using || or ??)
  • fix types

but found it out of scope. All call sites I looked at where not properly typed, and the tests where still js files.

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #956 (927c442) into develop (66daafe) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 927c442 differs from pull request most recent head 77c6d82. Consider uploading reports for the commit 77c6d82 to get more accurate results

@@             Coverage Diff             @@
##           develop     #956      +/-   ##
===========================================
- Coverage    36.49%   36.41%   -0.08%     
===========================================
  Files          499      466      -33     
  Lines         9425     9149     -276     
  Branches      1932     2002      +70     
===========================================
- Hits          3440     3332     -108     
+ Misses        5699     5368     -331     
- Partials       286      449     +163     
Flag Coverage Δ
unittests 36.41% <ø> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...Map/components/WelcomeModal/GridChilds/Column3.tsx 80.00% <0.00%> (-3.34%) ⬇️
src/components2/Article/ArticleWrapper/TOC.tsx 82.60% <0.00%> (-2.40%) ⬇️
src/components2/BaseMap/BaseMap.tsx 81.81% <0.00%> (-1.52%) ⬇️
.../components2/Article/ArticleHeader/ArticleMeta.tsx 93.75% <0.00%> (-0.70%) ⬇️
src/apps/Map/map-utils.ts 78.21% <0.00%> (-0.43%) ⬇️
src/routes.tsx 0.00% <0.00%> (ø)
src/AppState.ts 47.82% <0.00%> (ø)
src/apps/Map/index.tsx 0.00% <0.00%> (ø)
src/styles/fontfaces.ts 0.00% <0.00%> (ø)
src/apps/Gastro/utils.ts 33.66% <0.00%> (ø)
... and 165 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66daafe...77c6d82. Read the comment docs.

and fix linting errors. Directly editing via github webapp since fail to properly set it up locally for now (might be a webstorm thing)
@s-pic
Copy link
Collaborator Author

s-pic commented Jan 20, 2022

To test the test, enter the following geoJSON into https://geojson.io/

{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "type": "LineString",
        "coordinates": [
          [0, 0],
          [0, 5],
          [10, 5],
          [10, 10]
        ]
      }
    }]
}

@tordans tordans requested a review from ohrie January 21, 2022 10:45
@s-pic
Copy link
Collaborator Author

s-pic commented Jan 31, 2022

Sorry for letting this PR rot, still figuring out why along produces that result

@s-pic
Copy link
Collaborator Author

s-pic commented Feb 7, 2022

@tordans Not sure what to do here. Along seems to produce results close to what we should expect, but not exactly.
Did some research about alternative utils, without much success.

What I could do is to adapt test data once more so that lines with a smaller bounding box are fed into the test, since we are not interested in the util to work for large scale denominators.

@ohrie ohrie marked this pull request as draft March 28, 2022 08:05
@ohrie
Copy link
Contributor

ohrie commented Apr 4, 2022

Well I think that's a problem of turfjs itself, see Turfjs/turf#1362
Seems to be merged, but in the v7, which is not released since then ;) So I think that's not on your side, but we should wait for turfjs and in the meantime could check if the values are in a boundary of values.

Even in their official docs the function does obviously not work :D

@ohrie
Copy link
Contributor

ohrie commented Apr 4, 2022

Maybe it's fixed in the newest version 6.5.0 , since we didn't used it yet. I've updated it in #1012

@s-pic
Copy link
Collaborator Author

s-pic commented Apr 4, 2022

Alright, will try to sync the branch and re-run tests throughout the week

@ohrie
Copy link
Contributor

ohrie commented Apr 13, 2022

Actually the backport from develop didn't solve the problem :/ So this needs some more investigation.

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.

Add test for function reliant on turf dependency
2 participants