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

Browserify #9

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Browserify #9

wants to merge 10 commits into from

Conversation

jasonkarns
Copy link
Contributor

do not merge

There are multiple components to this PR, and some of them will be extracted as separate PRs (since some changes aren't specific to browser functionality)

  • the ui methods should be attached to the provided suite context, not global (this can be a separate PR. it is a standalone change)
  • the pre-require and post-require events are given a null value for file when invoked in the browser, so a default dummy value of 'browser' is used
  • the processedFiles.length guard will not work in the browser since mocha.files will be empty. However, I'm not sure of the purpose of this guard in node, so some alternative is necessary before this can be merged. For now, I've simply removed the guard and allowed each processedFile to be built into a mocha suite.
  • this PR requires that the post-require hook be invoked manually just prior to mocha.run(). For my testing, I have configured this manually, but I now have an open PR to Mocha which will do this automatically in the browser-distribution of mocha
  • browserify is required and configured via an npm script (npm run build) which drops the browser-build under /dist (this dist file will either need to be included in the npm tarball on prepublish, or committed in github, or published to github as a release asset. I can configure another PR that does this automatically whenever a new version is released)
  • coffeeify transform is configured for browserify to compile coffee into JS during the build step
  • browserify-shim transform is included such that the require('mocha') call is replaced with a window.Mocha shim. It should be assumed that if mocha-gwt is being used in the browser that mocha is included like a regular script and exists as the global Mocha (which is indeed what is done in the browser distribution of mocha)

@jasonkarns jasonkarns mentioned this pull request Sep 25, 2015
@TheLudd
Copy link
Owner

TheLudd commented Sep 26, 2015

However, I'm not sure of the purpose of this guard in node

The purpose was to allow ddescribe and xdescribe across files. Mocha previously had very bad support for this, I don't know if it has improved. Basically, mocha-gwt will read all files and register whatever they have and then build a single test suite, when that is complete it will construct a suite and add it to mocha. Your removal of the if statement breaks a test.

@jasonkarns
Copy link
Contributor Author

Will mocha.files ever be 0 in node? If not, we could use that as an
alternative predicate to the condition, since it is always 0 in the browser.
On Sep 26, 2015 4:25 AM, "Ludwig Magnusson" [email protected]
wrote:

However, I'm not sure of the purpose of this guard in node

The purpose was to allow ddescribe and xdescribe across files. Mocha
previously had very bad support for this, I don't know if it has improved.
Basically, mocha-gwt will read all files and register whatever they
have and then build a single test suite, when that is complete it will
construct a suite and add it to mocha. Your removal of the if statement
breaks a test.


Reply to this email directly or view it on GitHub
#9 (comment).

@TheLudd
Copy link
Owner

TheLudd commented Sep 26, 2015

No it will be the name of the test file required

@jasonkarns
Copy link
Contributor Author

This PR is now only blocked by mochajs/mocha#1905

@TheLudd
Copy link
Owner

TheLudd commented Sep 28, 2015

Ok I see the tests pass. But you say it will not work in the browser is that correct?

Could you rebase master? I added Travis integration just now

@jasonkarns
Copy link
Contributor Author

Correct, this PR will not work yet, until the PR against mocha itself is
merged (which ensures the post-require hook get executed in the browser).

I didn't bother adding any browser-specific tests yet, as I'm not sure what
you'd like to do. We can create a single (or just a few) integration-like
tests that execute in the browser. Or we can browserify the entire existing
test suite and run it in the browser in addition to node. Or we can do
both: get the unit test suite running in browser, as well as a browser
integration test. What are you thoughts? I can spike a PR later today to
experiment what it would look like if the existing test suite were to run
in the browser (in addition to node, of course).

@TheLudd
Copy link
Owner

TheLudd commented Sep 28, 2015

Yes tests for this would be needed. Apart from verifying the functionality they could also serve as a reference for how to set it up in the browser.

But generally, I would not like to merge it until it actually works with mocha

@jasonkarns
Copy link
Contributor Author

If you're looking for the browser tests to act as a reference, then it
wouldn't make sense to browserify the unit test suite. I'll focus on
creating just an integration test suite that's browser-only.

Definitely agree that this shouldn't be merged until mocha is updated. I'm
considering this blocked until then.

On Mon, Sep 28, 2015 at 10:14 AM, Ludwig Magnusson <[email protected]

wrote:

Yes tests for this would be needed. Apart from verifying the functionality
they could also serve as a reference for how to set it up in the browser.

But generally, I would not like to merge it until it actually works with
mocha


Reply to this email directly or view it on GitHub
#9 (comment).

@jasonkarns
Copy link
Contributor Author

Waiting for mocha to reopen the PR

mochajs/mocha#1905

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