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

Improve DOM-based integration tests #1247

Closed
Tyriar opened this issue Jan 25, 2018 · 2 comments · Fixed by #2068
Closed

Improve DOM-based integration tests #1247

Tyriar opened this issue Jan 25, 2018 · 2 comments · Fixed by #2068
Assignees
Labels
type/debt Technical debt that could slow us down in the long run
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jan 25, 2018

Right now we're on jsdom which has a lot of problems and as a result we don't write that many integration tests. https://github.com/GoogleChrome/puppeteer

@Tyriar Tyriar added the type/debt Technical debt that could slow us down in the long run label Jan 25, 2018
@bgw
Copy link
Member

bgw commented Mar 28, 2018

At $dayjob, we use a combination of JSDom and Puppeteer.

Puppeteer is definitely a lot more powerful, but it's also slower, and it's harder to get tests to run reliably. Puppeteer has an async API (it's easy to introduce race conditions) and more stuff that can go wrong. As a result, we've only built tests using Puppeteer for our most important surfaces.

I'd recommend using a combination of both, or just sticking with JSDom.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 28, 2018

@bgw I think your idea of adding node-canvas sounds like the most attractive first step atm

@Tyriar Tyriar changed the title Investigate using puppeteer for proper integration tests Improve DOM-based integration tests Mar 28, 2018
bgw added a commit to bgw/xterm.js that referenced this issue Mar 29, 2018
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 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 added a commit to bgw/xterm.js that referenced this issue Apr 18, 2018
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 added a commit to bgw/xterm.js that referenced this issue Apr 18, 2018
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).
Tyriar added a commit to Tyriar/xterm.js that referenced this issue May 11, 2019
@Tyriar Tyriar added this to the 3.14.0 milestone May 11, 2019
@Tyriar Tyriar assigned Tyriar and unassigned bgw May 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/debt Technical debt that could slow us down in the long run
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants