Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

MVP/pre-release#1

Open
usergenic wants to merge 21 commits into
masterfrom
third-rewrites-a-charm
Open

MVP/pre-release#1
usergenic wants to merge 21 commits into
masterfrom
third-rewrites-a-charm

Conversation

@usergenic

@usergenic usergenic commented Oct 11, 2019

Copy link
Copy Markdown
Contributor

This package provides a window.suiteChild method and patches mocha.run to enable running external HTML resources with their own mocha test suites and aggregating the results in the local mocha runner event-stream.

This was written when switching https://github.com/Polymer/lit-element test runner from web-component-tester to karma and identified that we needed a way to run the test suite with multiple permutations of polyfills in multiple window contexts without running multiple invocations of karma, which would massively balloon the runtime of the test suite.

The README does a basic job of showing usage as does everything in ./test/

Comment thread src/stats-collector.ts Outdated
'use strict';
/**
(The MIT License)
Copyright (c) 2011-2018 JS Foundation and contributors, https://js.foundation

@aomarks aomarks Oct 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fix copyright header

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So, this file was copied from mocha source and then edited to include typings. The createStatsCollector function isn't exported by the published npm package, so I had to copy it. I put a comment below the license header... maybe I should have put it above the license header?

**
 * The following was extracted from
 * https://github.com/mochajs/mocha/blob/master/lib/stats-collector.js
 */

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe I can skip the function and just make these properties part of the runner. That'd be cleaner. Will do.

@aomarks aomarks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for delay. A few initial questions, I'll take a closer look at the code next round.

Comment thread README.md
@@ -0,0 +1,39 @@
# mocha-suite-child

Intended for use when running mocha in web-browsers, runs mocha test suites defined in HTML documents and include the results as if they are part of the main context.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and includes

Comment thread README.md
// Defines the children of the current mocha context. Note that
// the same test suite files can be reused as children, with different
// configurations. These must be defined at the top-level, i.e.
// they can't be nested within Mocha's own `suite()` declarations.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add something here about how it creates an iframe for each suiteChild?

Comment thread src/controller.ts
container = document.createElement('div');
container.id = this.containerId;
container.style.position = 'absolute';
container.style.top = '-9999px';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this mean the iframe is not visible? If not, isn't it better for it to be visible so that you can see what it's doing, and because that could maybe affect throttling/in-view behaviors (e.g. requestAnimationFrame can be disabled for windows that are not in view).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants