sports-alliance / sports-lib

A Library for processing GPX, TCX, FIT and JSON files from services such as Strava, Movescount, Garmin, Polar etc
GNU Affero General Public License v3.0
149 stars 21 forks source link

Increase robustness via integrations tests #39

Closed thomaschampagne closed 4 years ago

thomaschampagne commented 4 years ago

This PR includes:

warning: Unit tests are in failure for some GPX tests. It's normal. Problem is about wrong streams sizes :). Indeed GPX files don't hold the distance value on first sample, giving a "size - 1" compared to hr stream for instance. I fixed the problem in a commit of this PR (https://github.com/jimmykane/sports-lib/commit/34482bbaf8833a6a5184e0f4332f410048e97bdc). But prefered to let the problem back to have your advices & decisions about it (cancel commit: https://github.com/jimmykane/sports-lib/commit/caaba6673a29be72ffc921c99da31442b0369083)

jimmykane commented 4 years ago

@thomaschampagne if you want you can revert that commit (that will fix the test).

In all my knowledge it sounds ok ti initiate the distance to 0. Even for "indoors" this should not matter.

thomaschampagne commented 4 years ago

@jimmykane Ok.

Before doing that, another solution could be to drop the first sample on all streams (when gpx parsing). Just brainstorming / debate...

codecov-io commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@caaba66). Click here to learn what that means. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #39   +/-   ##
=========================================
  Coverage          ?   71.69%           
=========================================
  Files             ?      172           
  Lines             ?     4239           
  Branches          ?      641           
=========================================
  Hits              ?     3039           
  Misses            ?     1185           
  Partials          ?       15
Impacted Files Coverage Δ
src/events/utilities/event.utilities.ts 66.29% <100%> (ø)

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 caaba66...72060ae. Read the comment docs.

jimmykane commented 4 years ago

If from your tests the 1st sample has other data nah, add 0.
Adding (I think) a 0 for start is safe. Adding other numbers where they should not be is debatable. :-)

jimmykane commented 4 years ago

FYI (and just saying) the lib I just fire it up on production on every little change. My users can easily repair data errors and I have a beta etc. So I do also do user tests with Sentry before I say it's ok. :-)

Just saying in case you also see some "emergency" commit / PR

thomaschampagne commented 4 years ago

OK ;).

Damn i missed a note i wanted to share with you about "distance = 0" if re-enabled...

If first sample has "distance = 0", then speed is "0" of course... But the first sample if pace stream is "Infinity"... Is this a problem for you? If yes i can provide the fix you need.

jimmykane commented 4 years ago

@thomaschampagne not a prob.

My implementation (on my app side) and line of thinking is this (correct me if wrong):

anyways good to go and no worries! Lets roll this fast and sweet and get a good lib out there.

FYI I am in contact and maintain a FIT file parsing decoder in JS (which is hard todo) but also the original writer is a good contact.