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.

Sign up for GitHub

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

Jump to bottom

Using delay in setup() depends on object property ordering #1799

Closed
jRiest opened this issue Jul 14, 2015 · 8 comments
Closed

Using delay in setup() depends on object property ordering #1799

jRiest opened this issue Jul 14, 2015 · 8 comments
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" status: accepting prs Mocha can use your help with this one! type: bug a defect, confirmed by a maintainer

Comments

@jRiest
Copy link

jRiest commented Jul 14, 2015

When trying to set {delay: true} in a mocha.setup() call, it only works if the delay property is set before the ui property.

Here's some reproduction steps for Chrome.

<!doctype html>
<html>
  <head>
    <link rel="stylesheet" href="mocha.css" />
  </head>
  <body>
    <div id="mocha"></div>
    <script src="mocha.js"></script>
    <script>
      window.onload = function() {
        // This works
        mocha.setup({delay: true, ui: 'bdd'});

        // This doesn't
        // mocha.setup({ui: 'bdd', delay: true});


        setTimeout(function() {
          describe('a test', function() {
            it('works', function() {});
          });
          run();
        }, 500);
        mocha.run();
      };
    </script>
  </body>
</html>

In the case where it doesn't work, the global run() method is undefined.

The issue is that mocha.setup() just iterates over the properties and calls their corresponding methods

for (var opt in opts) this[opt](opts[opt]);

And in the ui method, the pre-require event is emitted, and it's in those event handlers that the global "run" method is supposed to be registered:

context.run = mocha.options.delay && common.runWithSuite(suite);

However, if the ui method is called before the delay method, then mocha.options.delay will not be set and the global run method will not be defined.
@jRiest jRiest changed the title Using delay in setup() depends on property ordering Using delay in setup() depends on object property ordering Jul 14, 2015
@jRiest jRiest mentioned this issue Jul 14, 2015
Closed
@danielstjules danielstjules mentioned this issue Apr 27, 2016
Closed
@hollomancer hollomancer added the status: accepting prs Mocha can use your help with this one! label Aug 28, 2016
@jRiest
Copy link
Author

jRiest commented Aug 31, 2016

@hollomancer I think #1800 should fix this issue and all comments were addressed, but the pr-needs-work label was never removed. If there is something else that needs to be done, please let me know and I'll be happy to make the changes.

@akosyakov akosyakov mentioned this issue Nov 24, 2016
Closed
@stale
Copy link

stale bot commented Oct 17, 2017

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

@stale stale bot added the stale this has been inactive for a while... label Oct 17, 2017
@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" and removed stale this has been inactive for a while... status: accepting prs Mocha can use your help with this one! labels Oct 18, 2017
@boneskull
Copy link
Member

PR needs review again

@jljorgenson18
Copy link

Any updates on this? This is still an issue for karma-mocha and it should be an easy fix.

@plroebuck
Copy link
Contributor

plroebuck commented Dec 14, 2018 •
edited 

mocha.setup = function(opts) {
  if (typeof opts === 'string') {
    opts = {ui: opts};
  }

  Object.keys(opts).sort().forEach(function(opt) {
    if (opts.hasOwnProperty(opt)) {
      this[opt](opts[opt]);
    }
  }
  return this;
};

@boneskull boneskull added the status: accepting prs Mocha can use your help with this one! label Dec 14, 2018
@boneskull
Copy link
Member

easy fix, more difficult to test without refactors. and yes, we need a real test. #1800 added a test, but not an automated one.

@craigtaub
Copy link
Contributor

craigtaub commented Jun 16, 2020 •
edited

Sounds like there are 2 bugs right now in the browser usage.

  1. {delay: false} enables the delay
  2. the bug from this issue

Perhaps something like below to fix both issues?

if ('delay' in opts && opts['delay'] === true) {
  this.delay();
}
var self = this;
Object.keys(opts)
  .filter(function(opt) {
    return opt !== 'delay';
  })
  .forEach(function(opt) {
    if (Object.prototype.hasOwnProperty.call(opts, opt)) {
      self[opt](opts[opt]);
    }
  });

@boneskull What level of testing would we need with this do u think? AFAIK html reporter tests are not ready, could introduce a unit-level somewhere.

@craigtaub craigtaub mentioned this issue Jun 22, 2020
Merged
@craigtaub
Copy link
Contributor

This should now be fixed in the latest PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" status: accepting prs Mocha can use your help with this one! type: bug a defect, confirmed by a maintainer
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@boneskull @jRiest @craigtaub @jljorgenson18 @hollomancer @plroebuck