Large parts of leaflet-holtine are black

fschn90 commented 1 year ago

Subject of the issue

hotline doesnt seem to work. large parts are black instead of typical hotline colour patterns.

Your environment

leaflet-elevation: 2.4.0
leaflet: 1.9.4
browser: Firefox 113.0.1 (64-bit)
operating system: Ubuntu 22.04.2 LTS
link to your project: https://flo-schn.github.io/elevation-issue-v3/

Steps to reproduce

please see link to my project.

Expected behaviour

fully hotlined colour patterns of gpx line on map. as it worked properly before the last update ~ 2month ago.

Actual behaviour

large parts of it are filled black. somehow zooming in and out seems to affect how much of the line is black. see foto:

Screenshot from 2023-05-29 15-38-02

Raruto commented 1 year ago

Version 2.3.2 broke this (👉 https://github.com/Raruto/leaflet-elevation/commit/f27ae4c13c45773a36b808da22e5ccda55a057db)

Temporarily you can fix it by reverting that changes partially, like so:

<!-- togeojson (4.6.0) -->
<script src="https://unpkg.com/@tmcw/togeojson@4.6.0/dist/togeojson.umd.js"></script>

<!-- leaflet-elevation (v2.4.0) -->
<script src="https://unpkg.com/@raruto/leaflet-elevation@2.4.0/dist/leaflet-elevation.js"></script>

👋 Raruto

fschn90 commented 1 year ago

okay cool. thx for the info! is there a rough timeline for a fix? :grin:

Raruto commented 1 year ago

@flo-schn here is the reason behind this bug 👉 https://github.com/placemark/togeojson/pull/110

Please do some testing with togeojson@5.6.2 and then submit a pull request which updates the following lines accordingly:

https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/src/control.js#L17

https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/package.json#L69

👋 Raruto

fschn90 commented 1 year ago

Thanks for the opportunity to contribute. I'm still at an early stage of coding and thus unfortunately have a few noob questions but would really like to contribute. I hope thats okay, if u'r too busy then thats fine.

I followed these steps: https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/.github/CONTRIBUTING.md?plain=1#L26-L30

however encountered following error:

npm test

> @raruto/leaflet-elevation@2.4.0 test
> uvu . ^(examples)?(src)?\.*\.spec\.js$

sh: 1: Syntax error: "(" unexpected

i googled but didnt find a solution/fix. i'm also not entirely sure what i'm looking for, i suppose.

Question: Have you got any clues, leads or potentially even a solution?

Also, i'm not sure how to add tests: https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/.github/CONTRIBUTING.md?plain=1#L32-L32

I did test the temporary fix on 14 different gpx files and all worked well.

<!-- togeojson (4.6.0) -->
<script src="https://unpkg.com/@tmcw/togeojson@4.6.0/dist/togeojson.umd.js"></script>

<!-- leaflet-elevation (v2.4.0) -->
<script src="https://unpkg.com/@raruto/leaflet-elevation@2.4.0/dist/leaflet-elevation.js"></script>

but i guess thats not the kind of test you are looking for. Thus:

Question: How do i add a test? An additional html file in the examples folder with a hotlined gpx track that in the leaflet-elevation: 2.4.0 is displayed black? and with your updated lines should display the hotline just fine.
Question: Do you have any guidelines / schema / recommended reading on how to add a test / what constitutes a good test?

Really hope i'm not asking for too much. Thank you very much for your work! Best, Flo

Raruto commented 1 year ago

I encountered following error:

npm test

> @raruto/leaflet-elevation@2.4.0 test
> uvu . ^(examples)?(src)?\.*\.spec\.js$

sh: 1: Syntax error: "(" unexpected

That expression probably contains characters that should be escaped on your unix operating system (eg. missing quotes? uvu . "^(examples)?(src)?\.*\.spec\.js$")

Try searching if there are specific indications regarding the use of command line regexps (eg. bash node, npm..) for your operating system. 👉 See also: lukeed/uvu

I did test the temporary fix on 14 different gpx files and all worked well.


<!-- togeojson (4.6.0) -->
<script src="https://unpkg.com/@tmcw/togeojson@4.6.0/dist/togeojson.umd.js"></script>

<!-- leaflet-elevation (v2.4.0) -->
<script src="https://unpkg.com/@raruto/leaflet-elevation@2.4.0/dist/leaflet-elevation.js"></script>

ok, maybe you meant? (v5.6.2)

<!-- togeojson (5.6.2) -->
<script src="https://unpkg.com/@tmcw/togeojson@5.6.2/dist/togeojson.umd.js"></script>

<!-- leaflet-elevation (v2.4.0) -->
<script src="https://unpkg.com/@raruto/leaflet-elevation@2.4.0/dist/leaflet-elevation.js"></script>

How do i add a test?

This command:

uvu . ^(examples)?(src)?\.*\.spec\.js$

searches for and executes any *.spec.js files located within these folders:

.
├── examples/      # "frontend" tests (ie. full blown html pages)
│   ├── ...
│   └── *.spec.js
└── src/           # "unit" tests (ie. specific javascript files)
    ├── ...
    └── *.spec.js

like these two, for example:

https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/src/utils.spec.js#L1-L50

https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/examples/leaflet-elevation_clear-button.spec.js#L1-L76

FYI, tests within the example folder make also use of Playwright library so that they can be run in a browser

what constitutes a good test?

It's not fancy code, just figure out what are the assertions test, for example:

// <ele>0</ele> values shouldn't be interpreted as an empty ("false" or "undefined") value.

toGeoJSON.gpx(new DOMParser().parseFromString(
`<?xml version="1.0" encoding="UTF-8"?>
<gpx version="1.0" xmlns="http://www.topografix.com/GPX/1/0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <trk>
    <trkseg>
      <trkpt lat="45.763340000" lon="12.843770000">
        <ele>-1.000</ele>
      </trkpt>
      <trkpt lat="45.763240000" lon="12.847620000">
        <ele>0</ele>
      </trkpt>
    </trkseg>
  </trk>
</gpx>`, "text/xml"))

Take also a look at this other pull https://github.com/placemark/togeojson/pull/110 to get an idea of how a "snapshot assertion" is implemented (although they don't use the same dependencies, the concept is quite similar between all "testing libraries").

BTW, it is not mandatory to use a snapshot, it depends a bit on what exactly you want to test.

👋 Raruto

fschn90 commented 1 year ago

Thank you very much for the explanations. it took me a little while until i got the regex right, and it turned out to be much simpler than first thought: uvu . '.*\.spec\.js$'

got me to these files:

./leaflet-elevation.spec.js
./leaflet-elevation_custom-summary.spec.js
./leaflet-elevation_clear-button.spec.js
./leaflet-elevation_multiple-maps.spec.js
./leaflet-elevation_edge-scale.spec.js
./leaflet-elevation_almost-over.spec.js

i adapted the package.json file and was able to run the test. unfortunately not all tests passed:

  Total:     10
  Passed:    7
  Skipped:   0
  Duration:  18057.49ms

i'm not sure what the reason is. a common 'theme' seems to be: Failed to load resource: the server responded with a status of 404 ()

maybe thats connected to import { suite } from '../test/setup/http_server.js'; and in thurn with playwright?

i hope thats not too boring for you. i attached the entire npm test output at the end.

these are my installed node_modules:

npm list
@raruto/leaflet-elevation@2.4.0 ./leaflet-elevation
├── @rollup/plugin-commonjs@24.1.0
├── @rollup/plugin-node-resolve@15.0.2
├── @rollup/plugin-terser@0.4.1
├── @tmcw/togeojson@5.6.0
├── d3@7.8.4
├── http-server@14.1.1
├── jsdom@21.1.1
├── leaflet-i18n@0.3.1
├── leaflet@1.7.1
├── npm-run-all@4.1.5
├── playwright@1.32.3
├── postcss-copy@7.1.0
├── postcss-import@15.1.0
├── postcss@8.4.23
├── rollup-plugin-postcss@4.0.2
├── rollup@3.21.0
└── uvu@0.5.6

maybe thats also helpful info:

npm version
{
  '@raruto/leaflet-elevation': '2.4.0',
  npm: '9.5.1',
  node: '18.16.0',
  acorn: '8.8.2',
  ada: '1.0.4',
  ares: '1.19.0',
  brotli: '1.0.9',
  cldr: '42.0',
  icu: '72.1',
  llhttp: '6.0.10',
  modules: '108',
  napi: '8',
  nghttp2: '1.52.0',
  nghttp3: '0.7.0',
  ngtcp2: '0.8.1',
  openssl: '3.0.8+quic',
  simdutf: '3.2.2',
  tz: '2022g',
  undici: '5.21.0',
  unicode: '15.0',
  uv: '1.44.2',
  uvwasi: '0.0.15',
  v8: '10.2.154.26-node.26',
  zlib: '1.2.13'
}

If you have some clues, then that would again be greatly appreciated. Hope its not too cumbersome. I'd be really happy to contribute and also learn something along the way. but if time is short and its simply easier to fix yourself, i also understand.

Thanks again & best, Flo p.s.: my operating system is Ubuntu 22.04.2 LTS

p.p.s.: here the entire output:

npm test

> @raruto/leaflet-elevation@2.4.0 test
> uvu . '.*\.spec\.js$'

examples/leaflet-elevation_almost-over.spec.js
 examples/leaflet-elevation_almost-over.html  Failed to load resource: the server responded with a status of 404 ()
Error: <rect> attribute width: A negative value is not valid. ("-410.98113104939824")
Error: <rect> attribute width: A negative value is not valid. ("-410.98113104939824")
•   (1 / 1)

examples/leaflet-elevation_clear-button.spec.js
 examples/leaflet-elevation_clear-button.html  Failed to load resource: net::ERR_CONNECTION_RESET
Failed to load resource: net::ERR_CONNECTION_RESET
Failed to load resource: net::ERR_EMPTY_RESPONSE
✘   (0 / 1)

   FAIL  examples/leaflet-elevation_clear-button.html  "eledata_clear"
    page.evaluate: ReferenceError: controlElevation is not defined
    at eval (eval at evaluate (:195:30), <anonymous>:2:5)
    at new Promise (<anonymous>)
    at eval (eval at evaluate (:195:30), <anonymous>:1:15)
    at UtilityScript.evaluate (<anonymous>:197:17)
    at UtilityScript.<anonymous> (<anonymous>:1:44)

    at eval (eval at evaluate (:195:30), <anonymous>:2:5)
    at new Promise (<anonymous>)
    at eval (eval at evaluate (:195:30), <anonymous>:1:15)
    at UtilityScript.evaluate (<anonymous>:197:17)
    at UtilityScript.<anonymous> (<anonymous>:1:44)
    at load_trace (file:///home/fschn/Documents/Projects/leaflet-elevation/examples/leaflet-elevation_clear-button.spec.js:66:21)
    at file:///home/fschn/Documents/Projects/leaflet-elevation/examples/leaflet-elevation_clear-button.spec.js:13:26
    at Object.handler (file:///home/fschn/Documents/Projects/leaflet-elevation/test/setup/http_server.js:70:9)
    at Number.runner (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/dist/index.mjs:78:16)
    at async Module.exec (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/dist/index.mjs:141:33)
    at async Module.run (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/run/index.mjs:12:2)
    at async /home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/bin.js:26:5

examples/leaflet-elevation_custom-summary.spec.js
 examples/leaflet-elevation.html  Failed to load resource: the server responded with a status of 404 ()
•   (1 / 1)

examples/leaflet-elevation_edge-scale.spec.js
 examples/leaflet-elevation_edge-scale.html  Failed to load resource: net::ERR_CONNECTION_RESET
Failed to load resource: net::ERR_CONNECTION_RESET
Failed to load resource: net::ERR_EMPTY_RESPONSE
Failed to load resource: the server responded with a status of 404 ()
✘   (0 / 1)

   FAIL  examples/leaflet-elevation_edge-scale.html  "edge-scale"
    page.evaluate: ReferenceError: controlElevation is not defined
    at eval (eval at evaluate (:195:30), <anonymous>:2:24)
    at new Promise (<anonymous>)
    at eval (eval at evaluate (:195:30), <anonymous>:1:7)
    at UtilityScript.evaluate (<anonymous>:197:17)
    at UtilityScript.<anonymous> (<anonymous>:1:44)

    at eval (eval at evaluate (:195:30), <anonymous>:2:24)
    at new Promise (<anonymous>)
    at eval (eval at evaluate (:195:30), <anonymous>:1:7)
    at UtilityScript.evaluate (<anonymous>:197:17)
    at UtilityScript.<anonymous> (<anonymous>:1:44)
    at file:///home/fschn/Documents/Projects/leaflet-elevation/examples/leaflet-elevation_edge-scale.spec.js:11:50
    at Object.handler (file:///home/fschn/Documents/Projects/leaflet-elevation/test/setup/http_server.js:70:9)
    at Number.runner (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/dist/index.mjs:78:16)
    at async Module.exec (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/dist/index.mjs:141:33)
    at async Module.run (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/run/index.mjs:12:2)
    at async /home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/bin.js:26:5

examples/leaflet-elevation_multiple-maps.spec.js
 examples/leaflet-elevation_multiple-maps.html  Failed to load resource: the server responded with a status of 404 ()
•   (1 / 1)

examples/leaflet-elevation.spec.js
 examples/leaflet-elevation.html  Failed to load resource: net::ERR_CONNECTION_RESET
Failed to load resource: net::ERR_CONNECTION_RESET
Failed to load resource: net::ERR_EMPTY_RESPONSE
Failed to load resource: the server responded with a status of 404 ()
✘   (0 / 1)

   FAIL  examples/leaflet-elevation.html  "eledata_loaded"
    page.evaluate: ReferenceError: controlElevation is not defined
    at eval (eval at evaluate (:195:30), <anonymous>:2:7)
    at new Promise (<anonymous>)
    at eval (eval at evaluate (:195:30), <anonymous>:1:7)
    at UtilityScript.evaluate (<anonymous>:197:17)
    at UtilityScript.<anonymous> (<anonymous>:1:44)

    at eval (eval at evaluate (:195:30), <anonymous>:2:7)
    at new Promise (<anonymous>)
    at eval (eval at evaluate (:195:30), <anonymous>:1:7)
    at UtilityScript.evaluate (<anonymous>:197:17)
    at UtilityScript.<anonymous> (<anonymous>:1:44)
    at file:///home/fschn/Documents/Projects/leaflet-elevation/examples/leaflet-elevation.spec.js:11:28
    at Object.handler (file:///home/fschn/Documents/Projects/leaflet-elevation/test/setup/http_server.js:70:9)
    at Number.runner (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/dist/index.mjs:78:16)
    at async Module.exec (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/dist/index.mjs:141:33)
    at async Module.run (file:///home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/run/index.mjs:12:2)
    at async /home/fschn/Documents/Projects/leaflet-elevation/node_modules/uvu/bin.js:26:5

src/utils.spec.js
 src/utils.js  • • • •   (4 / 4)

  Total:     10
  Passed:    7
  Skipped:   0
  Duration:  18057.49ms

fschn90 commented 1 year ago

ok, maybe you meant? (v5.6.2)

<!-- togeojson (5.6.2) -->
<script src="https://unpkg.com/@tmcw/togeojson@5.6.2/dist/togeojson.umd.js"></script>

<!-- leaflet-elevation (v2.4.0) -->
<script src="https://unpkg.com/@raruto/leaflet-elevation@2.4.0/dist/leaflet-elevation.js"></script>

no. :D i actually did use togeojson (4.6.0) as suggested in your earlier comment:

Version 2.3.2 broke this (point_right f27ae4c)

Temporarily you can fix it by reverting that changes partially, like so:

<script src="https://unpkg.com/@tmcw/togeojson@4.6.0/dist/togeojson.umd.js"></script>


<script src="https://unpkg.com/@raruto/leaflet-elevation@2.4.0/dist/leaflet-elevation.js"></script>
wave Raruto

while togeojson (4.6.0) did work, i now changed it to v5.6.2 and all my 14 different gpx files are being displayed correctly :heavy_check_mark: :)

Raruto commented 1 year ago

Thank you very much for the explanations. it took me a little while until i got the regex right, and it turned out to be much simpler than first thought:
uvu . '.*\.spec\.js$'

Ok, but doing so I think you also perform a lookup search among all the other several files located within the node_modules folder.

i'm not sure what the reason is. a common 'theme' seems to be:
Failed to load resource: the server responded with a status of 404 ()
maybe thats connected to import { suite } from '../test/setup/http_server.js'; and in thurn with playwright?

Yes, end-to-end test reliability is likely not perfect.

For example, I noticed that running npm run dev in a separate terminal (before npm run test) decreased the error probability.

BTW, the main goals was to try to reuse the already available examples files in the folder in order to start writing some tests.

Essentially, for each test that file should do nothing but start the http-server (which is also used for development) and open the URL defined within test page (ref: the various /examples/*.spec.js files)

https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/test/setup/http_server.js#L8-L16

If it can be useful, in order to try to make a litte bit more reliable, I've read that some recommend using this function instead to wait for the page to load: pagewait for response

if time is short and its simply easier to fix yourself, i also understand.

I'm glad someone else tries to dig into the code a bit, so please take all your time, an extra head is always useful.

👋 Raruto

Raruto commented 1 year ago

this function could be promising as well: page wait for load state

fschn90 commented 1 year ago

Thanks for your reply.

regarding:

For example, I noticed that running npm run dev in a separate terminal (before npm run test) decreased the error probability.

leads to:

  Total:     10
  Passed:    8
  Skipped:   0
  Duration:  10577.90ms

however, i'm still getting plenty of Failed to load resource: the server responded with a status of 404 () which seem to be counted as passed tests..

i did look at:

pagewait for response and page wait for load state and did play around a little here like that:

export async function setup(ctx) { 
     ctx.server = new AbortController(); 
     exec('http-server', { signal: ctx.server.signal }); 
     ctx.localhost = 'http://localhost:8080'; 
     ctx.browser = await chromium.launch(); 
     ctx.context = await ctx.browser.newContext(); 
     ctx.context.route(/.html$/, mock_cdn_urls); 
     ctx.page = await ctx.context.newPage(); 
     ctx.page.waitForLoadState('domcontentloaded');
 }

not improving much. Thus me still being a javascript newbie and being completely new to playwright, i opted to simply implement your suggested changes in a pull request. stick with the current testing and not add any new ones.

@flo-schn here is the reason behind this bug point_right placemark/togeojson#110

Please do some testing with togeojson@5.6.2 and then submit a pull request which updates the following lines accordingly:

https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/src/control.js#L17

https://github.com/Raruto/leaflet-elevation/blob/e0c68cba9a71d140e4c5a4179c51ae34588b7327/package.json#L69

wave Raruto

as already mentioned above i tested togeojson@5.6.2 on my personal side on several gpx tracks all displaying the hotline fine.

hope that helps at least a little for new.

best, flo

Raruto commented 1 year ago

@flo-schn take a look at my latest edits in here 👉 https://github.com/Raruto/leaflet-elevation/pull/261

It's not perfect, but at least they don't crash all together...

👋 Raruto

fschn90 commented 1 year ago

i did npm run test, adapted to my setup with "test": "uvu . '.*\\.spec\\.js$'", in package.json, getting this test result:

  Total:     10
  Passed:    7
  Skipped:   0
  Duration:  17658.65ms

however, every html file in the example directory throws this error:

Failed to load resource: the server responded with a status of 404 ()
net::ERR_ABORTED https://unpkg.com/leaflet-ui@0.6.0/dist/locales/en-US.js

i did notice that in https://unpkg.com/browse/leaflet-ui@0.6.0/dist/locales/ there is no en-US.js file, but didnt find a link in the example html files to https://unpkg.com/leaflet-ui@0.6.0/dist/locales/en-US.js.

regarding your commits i also opened all the updated html files in the vscode Live Server and they seemed fine. if there is a better way to test, let me know.

Raruto commented 1 year ago

i did notice that in https://unpkg.com/browse/leaflet-ui@0.6.0/dist/locales/ there is no en-US.js file, but didnt find a link in the example html files to https://unpkg.com/leaflet-ui@0.6.0/dist/locales/en-US.js.

here is the explanation: https://github.com/Raruto/leaflet-gesture-handling/issues/20

regarding your commits i also opened all the updated html files in the vscode Live Server and they seemed fine.

Ok, let's say that this is enough for the moment (otherwise we go too far in time..).

In case of further improvements, feel free to open new pull requests.

👋 Raruto

Raruto / leaflet-elevation