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

[do not merge] dummy commit that shows CI doesn't check validity of html files #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timotheecour
Copy link
Contributor

@timotheecour timotheecour commented Jan 27, 2019

i had the suspicion so I created this PR.. turns out I was right, CI doesn't check validity of html files

running this locally you'll see a black window, dev console on chrome says:

Uncaught Error: No DOM element with id 'plot1' exists on the page.
    at Object.e.exports [as getGraphDiv] (plotly-latest.min.js:7)
    at Object.r.newPlot (plotly-latest.min.js:7)
    at x.html:11

but CI is green https://travis-ci.com/timotheecour/nim-plotly/builds/98718821

so we need something smarter to avoid regressions.
doesn't have to be 100% correct, but right now it doesn't check anything beyond nim code being valid :)
I don't know much about this topic, here's off top of my head:

approach 1: html-proofer (dead end IMO)

export GEM_HOME=$HOME/.gem
gem install html-proofer
$GEM_HOME/bin/htmlproofer --check-html /tmp/nimplotly/D20190125T182937.html

disappointing: doesn't catch lots of errors when i insert them (catches some, but IMO it's not suitable, unless there's some magic options i haven't seen)

Running ["ImageCheck", "LinkCheck", "ScriptCheck"] on /tmp/nimplotly/D20190125T182937.html on *.html...
Checking 1 external link...
Ran on 1 file!
HTML-Proofer finished successfully.

approach 2: selenium

approach 3

for each test, make browser save image displayed on screen as png, compare against groundtruth; if it differs make test fail and save images generated, uploading it somewhere, then we check visually whether new result is ok, and update groundtruth images

  • important: groundtruth images are saved in a different repo
  • cons: may be too rigid tests / more work

approach 4 (maybe simplest, in combination w approach 3 maybe)

log js console and check logs show what we expect
https://stackoverflow.com/questions/19298420/log-javascript-console-into-a-log-file-with-firefox

[EDIT] approach 5 (workaround): generate 1 image containing all plots

this mitigates downsides of current approach which opens a ton of browser tabs, consumes lots of memory, and is harder to verify:
use #43 to generate subplots in a single window, and save the image
in CI, that image can be uploaded somewhere and a human can quickly verify if it looks ok

@Vindaar
Copy link
Member

Vindaar commented Jan 28, 2019

Yes, that's indeed the case and we're aware of it. It's valid criticism of course. But given the amount of contributions to this project I didn't feel the urge to overcome my laziness in order to do something about it.

About the different approaches:

  • Regarding 1: I agree, proofing it's valid Html doesn't seem useful to me. The Html is static after all. The interesting aspect is the JS code for plotly.
  • Reg. 2: I've read about selenium, but haven't spent any time trying to understand whether it can help for such a usecase.
  • Reg. 3: This is exactly what I had in mind as a first step too, after saving images directly was implemented. But the fact that one would have to update all images if a slight change to default settings is done, was the reason I was too lazy.
  • Reg. 4: That actually is a nice idea and I didn't consider it. If the browser is opened successfully and the JS console remains empty, the code should be valid plotly. I might take a closer look at that.

Over the Christmas holidays I actually started experimenting with a different way to proof correctness of the Plotly code. My idea was to look at the Plotly schema:
https://api.plot.ly/v2/plot-schema?sha1=%27%27
and validate the created JSON against that schema at compile time. That experimentation code is here:
https://github.com/Vindaar/MetaPlot
It's basically a macro to write Plotly or Vega-Lite and have the resulting JSON be checked at CT. For Vega-Lite I wrote a JSON Schema validator
https://github.com/Vindaar/JsonSchemaValidator
to validate the JSON properly. Unfortunately Plotly does not provide a JSON Schema. Without it I resorted to some special keys, which need to be replaced, because the actual code is nested etc. seen here:
https://github.com/Vindaar/MetaPlot/blob/master/metaplot.nim#L11-L18
It's really only experimentation, but I feel like if polished it would provide a much nicer way for CI.

@timotheecour
Copy link
Contributor Author

  • I added approach 5 above, PTAL; any simple idea for the part "that image can be uploaded somewhere" from within CI ? ie, we'd need a server that accepts a POST request to upload that image, to which CI has access to (maybe no permissions needed, it's not security critical)

My idea was to look at the Plotly schema

but, like the html validation approach, this won't catch all bugs; it's useful for unit-testing but not for end-to-end validation; I'm expecting this library to grow more features over time, which also means to javascript logic including things not captured by simply validating the schema

verify the end result seems more robust

@Vindaar
Copy link
Member

Vindaar commented Jan 30, 2019

  • I added approach 5 above, PTAL; any simple idea for the part "that image can be uploaded somewhere" from within CI ? ie, we'd need a server that accepts a POST request to upload that image, to which CI has access to (maybe no permissions needed, it's not security critical)

Sure, this would help somewhat with approach 3. I'd be happy with that for now. I'll comment over at #43 about the subplots.

My idea was to look at the Plotly schema

but, like the html validation approach, this won't catch all bugs; it's useful for unit-testing but not for end-to-end validation; I'm expecting this library to grow more features over time, which also means to javascript logic including things not captured by simply validating the schema

verify the end result seems more robust

Well, if it's a proper schema and the Json is valid under it, there shouldn't be any bugs left. The question is whether the plot actually looks like intended of course. But that's only possible by verifying the end result anyways.

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.

2 participants