TheLudd / mocha-gwt

Given When Then for mocha
5 stars 2 forks source link

Browserify #9

Open jasonkarns opened 8 years ago

jasonkarns commented 8 years ago

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)

TheLudd commented 8 years ago

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 commented 8 years ago

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" notifications@github.com 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 https://github.com/TheLudd/mocha-gwt/pull/9#issuecomment-143409698.

TheLudd commented 8 years ago

No it will be the name of the test file required

jasonkarns commented 8 years ago

This PR is now only blocked by https://github.com/mochajs/mocha/pull/1905

TheLudd commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 <notifications@github.com

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 https://github.com/TheLudd/mocha-gwt/pull/9#issuecomment-143754346.

jasonkarns commented 6 years ago

Waiting for mocha to reopen the PR

https://github.com/mochajs/mocha/pull/1905