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

Introduce tests & CI #15

Merged
merged 12 commits into from
Dec 13, 2018
Merged

Introduce tests & CI #15

merged 12 commits into from
Dec 13, 2018

Conversation

dy
Copy link
Member

@dy dy commented Nov 15, 2018

This PR introduces a showcase for baseline tests (pilot).
Now CI is available for gl-components (yay).
That should reduce complexity of test coverage for gl-based traces in plotly.

Also that makes gl-component tests look way less messy, in standard tape- fashion.
@etpinard @archmoj please see test.js, hope that will make further dev simpler and fix regressions (seems that palette work may cause some).

@dy dy changed the title Palette, CI & tests Introduce tests & CI Nov 15, 2018
@@ -17,8 +18,7 @@
],
"browserify": {
"transform": [
"glslify",
"bubleify"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. You no longer convert es6 -> es5?

Copy link
Member Author

@dy dy Nov 15, 2018

Choose a reason for hiding this comment

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

bubleify does not do async/await bublejs/buble#125, and seems there is no more maintenance.
I wished there was a way for browserify to indicate this transform for main export, but ignore that for running tests.
Probably I should include babelify. Or just simply rewrite the thing in ES5, that is not a lot of hassle. Rollup maybe?

@etpinard
Copy link
Member

etpinard commented Nov 15, 2018

npm test is working for me locally 🎉

@dy
Copy link
Member Author

dy commented Nov 15, 2018

@etpinard there is a way to bundle the package as well, without sacrificing the dependencies. That is not as elegant as just browserify with --ignore, but the latter is unfortunately broken for that use-case.

That adds:

  • ES5 compilation on prepublish
  • tree shaking, if required
  • Removes glslify and other browserify runtime transforms (plotly bundling gets faster)
  • Keeps shared dependencies instead of packing them to bundle (ignore option)
  • Enables target browser versions via browserslist option https://github.com/browserslist/browserslist

package.json Outdated
"build": "browserify test.js -g bubleify | indexhtmlify | metadataify | github-cornerify | mobilify > gh-pages/no-snapping.html"
"test": "node test",
"test-browser": "budo test -- -t glslify",
"prepublishOnly": "npm run buikd",
Copy link
Member

Choose a reason for hiding this comment

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

build

@etpinard
Copy link
Member

Well, that's annoying. How many more bytes is that going to result in plotly.js? Oh well, maybe plotly.js is already too big to care about bundling glslify twice.

@dy
Copy link
Member Author

dy commented Nov 15, 2018

@etpinard glslify does not get bundled, that just inlines glsl dependencies to a bundle. Now in plotly.js glslify does inlining whenever we run plotly build, but with that PR glslify is not required anymore, because glsl is inlined already, so plotly building gets faster.

Moreover, right now plotly building also runs bubleify, that performs the same transpilation internally. With that PR we cut that as well, by providing transpiled code.

More to that, with this PR glslify is not a production, but dev dependency, meaning that plotly.js package as a dependency is faster to run with browserify, and the component is quicker to install.

@dy
Copy link
Member Author

dy commented Nov 23, 2018

@etpinard should we proceed with that?

@etpinard
Copy link
Member

Looks like I was completely wrong in #15 (comment), building plotly.js off the latest commit on this branch results in a slightly smaller bundle:

image

That said, looks like this PR here breaks a bunch of plotly.js tests:

  • (plotly.js) $ npm run test-image-gl2d barfs, there are still some non ES5 tokens in the regl-scatter2d bundle.js that don't get along with our old nw.js image test server.
  • (plotly.js) $ npm run test-jasmine -- --tags=gl spits out a bunch of

image

errors.

  • (plotly.js) $ npm run test-syntax gives:

image

@dy dy mentioned this pull request Dec 11, 2018
@dy
Copy link
Member Author

dy commented Dec 11, 2018

Oh Lord, I've found that. regl-splom uses wrong regl-scatter2d entry, pointing to not a bundle.
@etpinard can you please check tests with gl-vis/regl-splom#10
They run fine on my side.

@etpinard
Copy link
Member

plotly.js tests are green, 💃

Thanks @dy !

@dy dy merged commit 9572b6f into master Dec 13, 2018
@dy dy deleted the palette branch December 13, 2018 02:48
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.

None yet

2 participants