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

this.timeout() fails when using ES6's arrow functions #2018

Closed
ibc opened this issue Dec 21, 2015 · 59 comments
Closed

this.timeout() fails when using ES6's arrow functions #2018

ibc opened this issue Dec 21, 2015 · 59 comments
Assignees
Labels
faq a frequently asked question or common misconception

Comments

@ibc
Copy link

ibc commented Dec 21, 2015

When using Node >= 4 with "use strict" and ES6 syntax for arrow functions, mocha fails:

describe('foo', () => {
  this.timeout(100);
});

# => TypeError: this.timeout is not a function

Using ES5 syntax does work:

describe('foo', function() {
  this.timeout(100);
});

So, which kind of ugly trick does mocha with this?

@danielstjules
Copy link
Contributor

It binds the function to the test context, which can't be done when using arrow functions. From http://mochajs.org/

screen shot 2015-12-21 at 8 06 34 am

Sorry about that!

@ibc
Copy link
Author

ibc commented Dec 21, 2015

Thanks.

Why so many "magic" that, at the end, produces problems? Why not this?:

var mocha = require('mocha');

mocha.describe('foo', (suite) => {
  suite.timeout(100);

  suite.it('must love bar', () => ... );  
});

No globals, no problematic magic... but just JavaScript.

@danielstjules
Copy link
Contributor

What you're proposing is a major breaking change and something that's being discussed in #1969 (comment) A rewrite might introduce those types of semantics :)

@ibc
Copy link
Author

ibc commented Dec 21, 2015

Nice to know. Thanks.

@ORESoftware
Copy link

@ibc well, it would have to be

var mocha = require('mocha');

mocha.describe('foo', (suite) => {
  suite.timeout(100);

  suite.it('must love bar', (suite) => ... );  
});

but are the two different suite arguments the same type? probably not, so then you have different variable names to reflect the different types like so

var mocha = require('mocha');

mocha.describe('foo', (suite) => {
  suite.timeout(100);

  suite.it('must love bar', (test) => ... );  
});

@boneskull
Copy link
Contributor

anyway, the arrow operator cannot be used in functions that expect contexts.

I don't envision breaking the BDD UI for #1969--right off the bat, anyway--though I could be persuaded. It was my hope that we keep the existing API, and introduce a separate package containing the BDD UI. Mocha will ship with a version of the BDD UI which uses the existing API, but then we can release a new version of the BDD UI package using lambdas thereafter--users can choose whether or not to explicitly upgrade to that package.

@leonyu
Copy link

leonyu commented Mar 2, 2016

Maybe alternative ES6 syntax for describe or suite wrapper might solve this problem:

describe({ feature: 'create stuff' , do () {
    it('abc', () => {
    }); 
}})

This would at least allow binding at the suite level.

@mroien
Copy link

mroien commented Apr 20, 2016

Any update on this? I have this

mocha = require('mocha');

mocha.describe('test', (suite) => {

suite.timeout(500);

suite.it('test', (done)=>
          )
    }
)

And getting TypeError: Cannot read property 'timeout' of undefined

@boneskull
Copy link
Contributor

@mroien this is not a Mocha bug. the arrow syntax is not a 1:1 replacement for function. please read up on its limitations

DmitryDorofeev added a commit to DmitryDorofeev/mocha that referenced this issue Jul 1, 2016
neilbmclaughlin added a commit to nhsuk-archive/connecting-to-services that referenced this issue Sep 27, 2016
Needed to change function from arrow function as they are not
supported for mocha tests which require the use of timeout()

See: mochajs/mocha#2018
neilbmclaughlin added a commit to nhsuk-archive/connecting-to-services that referenced this issue Sep 27, 2016
Needed to change function from arrow function as they are not
supported for mocha tests which require the use of timeout()

See: mochajs/mocha#2018
neilbmclaughlin added a commit to nhsuk-archive/connecting-to-services that referenced this issue Sep 27, 2016
Needed to change function from arrow function as they are not
supported for mocha tests which require the use of timeout()

See: mochajs/mocha#2018
neilbmclaughlin added a commit to nhsuk-archive/connecting-to-services that referenced this issue Sep 27, 2016
Needed to change function from arrow function as they are not
supported for mocha tests which require the use of timeout()

See: mochajs/mocha#2018
neilbmclaughlin added a commit to nhsuk-archive/connecting-to-services that referenced this issue Sep 27, 2016
Needed to change function from arrow function as they are not
supported for mocha tests which require the use of timeout()

See: mochajs/mocha#2018
neilbmclaughlin added a commit to nhsuk-archive/connecting-to-services that referenced this issue Sep 27, 2016
Needed to change function from arrow function as they are not
supported for mocha tests which require the use of timeout()

See: mochajs/mocha#2018
@SamStonehouse
Copy link

Did anything come of this? I like the proposed solution, if only for my love of arrow functions and aversion to 'this' when it's not required

@Munter Munter added the faq a frequently asked question or common misconception label Nov 22, 2016
@nomilous
Copy link

Since timeout is only relevant with done, why not simply attach the timeout function to the done function.

it('makes sense', done => {
    done.timeout(100);
});

@wmahad
Copy link

wmahad commented Dec 19, 2016

@nomilous this still doesn't work. I've had a similar problem. What works for my case is calling setTimeout inside the it block. e.g.

it('description', done => {
     const func = () => {
          // assertions
     };
     setTimeout(func, 10000);
});

@teameh
Copy link

teameh commented Dec 27, 2016

@nomilous synchronous or cases returning a promise can also have a timeout.

@andela-engmkwalusimbi this isn't supposed to work. As @boneskull wrote:

@mroien this is not a Mocha bug. the arrow syntax is not a 1:1 replacement for function. please read up on its limitations

@boneskull
Copy link
Contributor

boneskull commented Dec 28, 2016

For everyone still wondering about this, ensure you understand what an arrow function implies, then come back here and read on (there are plenty of resources out there that can explain this far better than I can).

The only way this arrow functions would work in this case is if we change the bdd API to pass a context object to every Runnable's (hooks, tests) callback, instead of leveraging this. That's not a bad idea, but it's an earthquake of a breaking change, so will never happen. Instead of this:

it('should do something', function (done) {
  this.timeout(9000);
  // stuff
  done();
});

it'd look like this:

it('should do something', (context, done) => {
  context.timeout(9000);
  done();
});

That'd would break every async Mocha test in existence, regardless if it was backwards-compatible otherwise:

it('should do something', function (context, done) {
  // context is unused, but 'done' is now the second parameter
  this.timeout(9000);
  done();
});

We could provide an alternative bdd implementation that does this, however--it just wouldn't be the default.

That's about the most thorough explanation I have of "where this issue is at". 😄

@SamStonehouse
Copy link

Maybe it could be taken into consideration for the next major version? I don't think it's an important enough change to create an alternative bdd implementation. Having named arguments might help with future development too and maybe create a simple way to add some kind of test middleware like so:

it('should do something', function ({ context, done }) { ...

@danielstjules
Copy link
Contributor

We could provide an alternative bdd implementation that does this, however--it just wouldn't be the default.

@boneskull A new bdd-es6 interface would be great :)

@steebchen
Copy link

Although I am in love with arrow functions which are really useful for e.g. Array functions like .filter(i => i.val), what's the problem with using normal functions? I think it's quite useful to have describe and it globally so I don't have to require them each time. Also since when is this magic, only because you don't understand (arrow) functions? I definitely don't want to provide a variable each time when I can return promises, otherwise I would have switched to something like ava a long time ago. Regarding the simplicity of mocha I think there shouldn't be any large change on normal/arrow functions described in #1969. And please don't tell me arrow functions are faster to type since your editor can transform a single f into function () {\n\t\n}.

@ralyodio
Copy link

ralyodio commented Jan 26, 2017

i'm unclear, is there a solution to timeout out a before() call that uses arrow functions?

    before( async function () {
      data = await provider.getData();
      console.log(data);
      this.timeout(30*1000);
    });

Has no effect. still getting 4 second timeout here. This is the only slow thing in my test suite. I can put timeout to 30s in mocha.opts to solve the problem, but I don't really need all tests to timeout after 30s just the one api call when 4s is fine for 99% of them.

Here's how I solved it in the meantime (note that the first describe() uses function instead of fat arrow syntax:

describe('Search API', function () {
	this.timeout(30*1000);

	context('search', () => {
		let data;

		before( async () => {
		  data = await provider.getSearch();
		});

		it('returns results', () => {
		  expect(data).to.exist;
		  expect(data.results).to.be.instanceOf(Array);
		  expect(data.results.length).to.be.above(0);
		});
	})
});

@thom-nic
Copy link

I swear I read somewhere you could do something like this:

describe('a slow thing', () => {
 // test code...
}).timeout(5000);

which doesn't alter the parameter contract to the provided function. Now I can't find any reference to any such thing so maybe I just imagined it.

@cornpops
Copy link

cornpops commented Nov 12, 2017

@thom-nic this works! i guess it makes sense since all of mocha's functions returns its context

return this;

@amado-saladino
Copy link

It works with me when replacing the arrow function form with the normal function

function() { ..... }

@astitt-ripple
Copy link

Hi Folks. I apologize for going dark after sparking the discussion back in august, i've been quite busy and actually have mostly completed/moved-on from that work.

I appreciate the detailed response about the different use cases and how its hard to dovetail them together. That was the most concise showing of the different setups mocha has to support that I've read. So thank you for your time on that one.

Looking back, its clear that I must have over-emphasized getting access to mocha context (this), when that aspect was really more of a convenient after-thought. I didn't realize how easily it would draw attention away from what I was actually trying to do: which was having some fun by adding a temporal'ish extension (when) to the test dsl for streamlining one-off timeout adjustments (plus eliminating a common error for a particular style of tests, which I'll explain below). Returning this was just another fun thing I thought of adding, the main idea (hence the name when) was for handling cases that needed different timeouts than normal.

Obviously, if I wanted to access the bound context I could simply use function directly as many have suggested, rather than hoisting it out with a wrapper. Thats not the issue. 😄 It isn't lost on me how that might seem strange on the face of it. I'm hoping maybe it will round out the picture if I show how I was setting up some of the tests which led me down this path to begin with. To be clear, I'm not trying to sell any particular style here, use what works for you. This is what worked for me.

Ok
First off, start with the assumption that we're testing some setup that basically does one thing, but will do it for a wide range of inputs, and so we must test this thing out in a bunch of scenarios to ensure the outputs are correct. However, since the relevant application code "does one thing", the underlying test procedure is pretty much always the same. I also don't want to duplicate/mutate the test body needlessly, since that slows down how quickly I can add more test cases for new inputs and eventually the maintenance would become unreasonable.

So instead we write a function thats general enough to start the application code up with any potentially supported inputs, perform the test action, and then assert the results... Add that in my case I was working with the Promise abstraction (for reasons that i'll leave out), so this generalized test procedure function naturally has to return that promise.then chain. Bread and butter es6 kind of stuff, so far so good.

Now comes the test scenarios, since we packed everything into our test-procedure function, the test cases are effectively defining the inputs and invoking the function.

So perhaps I write a bunch of tests like this, and everything seems to be working:

it('does something', function() {
  testProcedureFunction('something','1')
})

If you're following along closely, you will have probably noticed already that this example has a bug. It is missing its return, and since testProcedureFunction is built on promises (pun intended), its always going to pass no matter whether the assertions at the end passes or fails. This is a bug, and it can be an extremely subtle one to track down sometimes. To illustrate, depending on how we wrote testProcedureFunction and how the application is written, lets say there is some synchronous code at the start, and that blows up instead of the end-of-test assertions, the testcase might even fail -- leading us to think everything is fine.

The test should of course really look like this with a return:

it('does something', function() {
  return testProcedureFunction('something','1')
})

Now I know this test is going to be one line in most cases. Actually, every case will be one line, except for when the inputs are such that a larger timeout is required. Now among the differences between classical js functions and arrows, there is a particular aspect of arrow functions that is useful here: a single statement arrow function has an implied return when braces are omitted. If instead of writing a function {...}, a test uses the => ..., then I can easily scan those cases for the arrow and the lack of curly braces, and quickly infer that they cannot have this missing return issue, and it will take some extra steps (adding the braces back) for someone to mess that up.

Like so:

it('does something', () => testProcedureFunction('something','1'))

Now what if one of these cases takes longer than the others! We can of course set the timeout like this:

it('does something slow', function() {
  this.timeout(10000)
  return testProcedureFunction('somethingSlow','2')
})

Or maybe someone will make a mistake and do this first (which doesn't work of course):

it('does something slow', () => {
  this.timeout(10000)
  return testProcedureFunction('somethingSlow','2')
})

But now we're back where we started, the codebase has a pattern ripe for repeating, which is susceptible to the missing return bug (either by future me, or the next person to add a feature -- the point is, its an easy mistake to make, it may go unnoticed, and it can be hard to track down). The when function solves this, and lets us use arrows consistently again:

it('does something slow', when(() => testProcedureFunction('somethingSlow','2'), 10000))

(note, i wasn't able to get the .timeout(5000) dot-chaining suggestion above to work, may have been due to the version of mocha i needed to use, I don't remember anymore, will give that a try!)
(note2, notice that the use of when isn't using the this parameter hoisting trick -- it really was just an after-thought).

Maybe there are linters that can flag missing returns-for-promise bugs (or probably more realistically, enforcing a return statement with a rhs for every function). However that wasn't an option at the time, plus I think the arrow syntax comes out shorter and I find it (subjectively/personally) easier to read and work with, which tipped the scales for me away from function.

So there you have it.

I don't know if I'll have time to respond again anytime soon, so I hope that was at least informative and clear, and maybe even puts some of the controversy surrounding the whole "access to mocha-context from arrows" stuff to bed.

Lastly, since I found function vs => confusing for a long time, I'll drop this link in case its not clear to anyone casually reading why arrows can't access this. It was the clearest explanation of functions vs arrows that I've found, and was what helped me finally understand the differences well enough to use them with full confidence.

https://hacks.mozilla.org/2015/06/es6-in-depth-arrow-functions/

@aleung
Copy link

aleung commented Dec 20, 2017

@thom-nic It works on it, but not on describe.

describe('my test suite', () => {

  it('test case 1', () => {
    // ...
  }).timeout('10s');  // Works on `it`. This test case doesn't timeout

  it('test case 2', () => {
    // ...
  });  // This test case timeouts.

}).timeout('10s');  // Doesn't work on `describe`. Tests are already created when runs to here.

@amado-saladino
Copy link

@thom-nic , you may use the normal function form

describe('my test suite', function() {
this.timeout(n);

...
}

@Jakobud
Copy link

Jakobud commented Apr 20, 2018

Anyone complaining about this doesn't understand arrow functions.

Arrow functions are NOT a new fancy ES6 thing is supposed to replace the classic function () {}. The only purpose of arrow functions is that it inherits this from it's parent, where the classic function () has it's own this.

Yes, even when using full ES6 syntax, you should still be using function () if you want to use this in the correct context of your function. You should be using both function () and () => in your ES6 application depending on what you are trying to do.

this.timeout() doesn't work with it('....', () => { ... }) because the callback is inheriting this from the parent describe() function, in which this.timeout() doesn't make sense on that level.

@Flamenco
Copy link

Don't lambda functions also allow you to automatically send a single argument to the function without declaring it in the call?

(param)=>aFunction
...then(aFunction)

function(){} can be bound to 'this' but ()=> is 'locked in'

Arrow functions should replace the traditional function when the receiver expects a pre-determined 'this' in the same context that it is called from, (and also benefit from typing less code).

I would go so far as to say never use function() unless you want 'this' to be something other than what 'this' is when invoking it.

@codefoster
Copy link

@Flamenco...

Don't lambda functions also allow you to automatically send a single argument to the function without declaring it in the call?

I'm not sure I understand exactly how you're putting that.
As far as "sending arguments to the function", fat arrows work just like regular functions with a single exception: if you have exactly 1 argument, then you can leave off the parenthesis.

() => console.log("hi"); //zero arguments requires empty parenthesis
a => console.log(a); //you can optionally leave the parenthesis off for 1 argument
(a,b) => console.log(`${a} ${b}`); //2..n arguments requires parenthesis

What you may have been getting at is that fat arrows allow you to return a value by omitting the curly braces and the return keyword as long as your function is a single expression.
So if you had...

setTimeout(function(a,b) { doSomething(); return calculateSomething(a,b); }, 5000);

...and you wanted to convert that into a fat arrow function, you would not be able to shake the curly braces and the return keyword because the function body has multiple statements. You'd do it like this...

setTimeout((a,b) => { doSomething(); return calculateSomething(a,b); }, 5000);

If, rather you started with...

setTimeout(function(a,b) { return calculateSomething(a,b); }, 5000);

...then you're dealing with a function that so simple that it just returns a single expression and you could use...

setTimeout((a,b) => calculateSomething(a,b), 5000);

That got a lot easier to read!
I wrote a bit more about this at codefoster.com/levelup-arrays.

@leonyu
Copy link

leonyu commented Apr 21, 2018

There are many different coding styles in JavaScript -- from OOP to FP, from strict type-safety to mixin/duck-typing. In addition, there are advanced patterns in each of those styles (i.e. dependency injection in OOP camp, currying/monad in the FP camp).

If your coding style is closer to FP where this is not used and arrow functions are used to reduce boilerplate, having to preserve this is an extra overhead for any advanced testing (e.g. parameterized testing, creating DSL).

Any experienced developer can just pre-wrap the testing framework to suit their coding style, but that means the framework is less "out-of-the-box". That translates to extra work for upgrading, adopting plugins, and onboarding new engineers.

@boneskull
Copy link
Contributor

I like the idea of an alternative bdd interface which doesn’t use this and instead passes what would normally be the context object as a parameter to describe, it and hooks.

But it’s not so straightforward to implement, IIRC. Would be cool to see an attempt though.

@RobRendell
Copy link

RobRendell commented Aug 27, 2018

I know this is getting into serious side-effects land, but couldn't you deal with the done-or-context parameter something like this?

it("runs immediately", () => {
  // call functions and assert whatever
})
it("runs asynchronously", doneOrContext => {
  setTimeout(() => {
    // call functions and assert whatever
    doneOrContext();
  }, 100)
})
it("runs immediately using the parameter as a context", doneOrContext => {
  doneOrContext.configureByMutation("some value");
  // As well as changing config, also flags to Mocha that this test is treating the
  // parameter as a context object and is therefore not async.
  // Call functions and assert whatever
})
it("runs asynchronously using the parameter as a context", doneOrContext => {
  doneOrContext.configureByMutation("some value");
  doneOrContext.setAsync(); // Flags to Mocha that even though the parameter has been used as
  // a context object, the test is in fact asynchronous.
  setTimeout(() => {
    // call functions and assert whatever
    doneOrContext();
    // or doneOrContext.done()
  }, 100)
})

@selvakumar1994
Copy link

i used the below script but i got the same Timeout exceed error.

Myscript :

describe("getBillingDetail", async function (){
this.timeout(55000);
it.only("check given valid Jobname",async function (done){
this.timeout(55000);
var result = await url.getBillingDetail('12254785565647858');
console.log(result);
assert.equal(result,true);
});
});

Error: Timeout of 55000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

@Munter
Copy link
Contributor

Munter commented Aug 29, 2018

Don't pass a done callback to an async function

@papercuptech
Copy link

papercuptech commented Sep 21, 2018

I've created a prototype solution that's backwards compatible. For now it's a separate module, but functionality could easily be merged into mocha proper.

https://github.com/papercuptech/mocha-lambda

quick way

require('mocha-lambda')
// now a global '_tst' can be used where 'this' was before

describe('suite', () => {
  beforeEach(() => {
    _tst.answer = 42
  })

  it('provides "this" as "_tst"', function() {
    assert(this === _tst)
  })

  it('works', () => {
    assert(_tst.answer === 42)
    _tst.skip()
  })
})

for explicit naming (and works with TypeScript)

// if you were previously explicitly importing api (describe, it, etc.) from 'mocha',
// you will have to change to importing from 'mocha-lambda', until (or if) this
// gets merged into mocha proper
import ctx, {describe as d, it as i} from 'mocha-lambda'

d('suite', () => {
  // ctx() is a function that returns "this" as appropriate
  i('works using ctx()', () => {
    ctx().skip()
  })
})

import {Context} from 'mocha'
// ctx() can also rename global
ctx('t')
declare var t: Context
d('suite', () => {
  // ctx() is a function that returns "this" as appropriate
  i('works using renamed global', () => {
    t.skip()
  })
})

@aleung
Copy link

aleung commented Sep 21, 2018

@papercuptech The link is 404 not found.

@papercuptech
Copy link

papercuptech commented Sep 21, 2018

Woops.. was private repo. now public

Can also npm i mocha-lambda

@papercuptech
Copy link

@aleung @linesh-simplicity, this is superseded by #3485

@lastboy
Copy link

lastboy commented Jul 15, 2019

Thanks.

Why so many "magic" that, at the end, produces problems? Why not this?:

var mocha = require('mocha');

mocha.describe('foo', (suite) => {
  suite.timeout(100);

  suite.it('must love bar', () => ... );  
});

No globals, no problematic magic... but just JavaScript.

see @thom-nic answer, clean and did the trick

@polaroi8d
Copy link

I swear I read somewhere you could do something like this:

describe('a slow thing', () => {
 // test code...
}).timeout(5000);

which doesn't alter the parameter contract to the provided function. Now I can't find any reference to any such thing so maybe I just imagined it.

@thom-nic solution worked for me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
faq a frequently asked question or common misconception
Projects
None yet
Development

No branches or pull requests