Add node-canvas to help increase the testable surface #1392
Conversation
This is at least a first step towards fixing xtermjs#1247. This adds some complexity to the initial setup, since canvas needs to be built from source. This won't be true once canvas 2.x is stabilized, because that version downloads (Automattic/node-canvas#992). JSDom doesn't support 2.x, and doesn't plan to (jsdom/jsdom#1964) until a stable release is cut. We could use [canvas-prebuilt](https://github.com/node-gfx/node-canvas-prebuilt), which JSDom does appear to support. However, I'm not sure if that would cause pain for people with 32-bit operating systems (see the compatibility table on that page). I'm not sure if this travis configuration will work; I'll iterate on it in the PR if it fails. I removed a bunch of hacks that were added in xtermjs#690, since they don't look necessary anymore (the sleep module is gone).
bgw
changed the title
|
I've realized this will probably break the docker and procfile setups, so I'm going to try to test/fix those before landing. |
|
@bgw do you believe we can enforce this on Travis CI, while making it opt-in for the developers? In the test suite we can try to import node-canvas and in case it fails, just skip the test. Also, I think it's pretty straightforward to include the node-canvas dependencies in the Docker image. I can help with that, if you would like. |
There was a problem hiding this comment.
This won't be true once canvas 2.x is stabilized, because that version downloads (Automattic/node-canvas#992). JSDom doesn't support 2.x, and doesn't plan to (jsdom/jsdom#1964) until a stable release is cut.
If this is just a prebuilt variant of canvas and jsdom supports it then I saw we go with that. No need to install the deps and I don't think it matters much if we don't support running the tests on 32-bit machines (with a disclaimer in the readme).
In the test suite we can try to import node-canvas and in case it fails, just skip the test.
The dependency list might intimidate people so they may avoid trying out the demo. It would be better if this was split out into a testing section so it doesn't look like it's mandatory for running the demo.
Could we fail the test with a message if t's not there?
| @@ -4,12 +4,20 @@ os: | |||
| node_js: | |||
| - 6 | |||
| before_install: | |||
| - if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test ; fi | |||
| @@ -45,6 +45,7 @@ | |||
| "@types/node": "^6.0.41", | |||
| "@types/text-encoding": "0.0.32", | |||
| "browserify": "^13.3.0", | |||
| "canvas": "^1.6.10", | |||
There was a problem hiding this comment.
Does JSDom just pick this module up if it's available?
|
I think this one is obsolete at this point as we want to move away from jsdom and instead improve testing by moving things to a node only lib (and evaluate how we can better test UI stuff later). |
This is at least a first step towards fixing #1247. This adds some complexity to the initial setup, since canvas needs to be built from source.
This won't be true once canvas 2.x is stabilized, because that version downloads (Automattic/node-canvas#992). JSDom doesn't support 2.x, and doesn't plan to (jsdom/jsdom#1964) until a stable release is cut.
We could use canvas-prebuilt, which JSDom does appear to support. However, I'm not sure if that would cause pain for people with 32-bit operating systems (see the compatibility table on that page).
I'm not sure if this travis configuration will work; I'll iterate on it in the PR if it fails. I removed a bunch of hacks that were added in #690, since they don't look necessary anymore (the sleep module is gone).
(I previously had a copy of this PR up over at bgw#1)