Refactor async.queue behaviour affected by upgrade

[josebolos]

Fixes an issue with the cron job not launching any pa11y tasks when triggered.

[joeyciechanowicz]

I think that as runPa11yOnTasks is using async.queue which in turn is using setImmediate internally, the try/catch won't capture the error.

async/queue has a .error handler though. runPa11yOnTasks would need to be changed to return a Promise which the .error handler would reject. Then await runPa11yOnTasks in this try/catch.

http://caolan.github.io/async/v3/docs.html#queue

[joeyciechanowicz]

Given that this broke and we had no idea, we should really add some unit tests for this. It shouldn't be too bad as the app is passed in so the model can have task.getAll and task.runById mocked out.

We'd need a cron string to make it run every 1 sec or something. Probably wouldn't be the prettiest looking test....

[josebolos]

You're right in that this needs tests as none of this code is being tested at the moment, which is why we didn't realise it's broken.

That said, we're trying to get a fix released ASAP as dashboard is broken because of this so we probably want to add the tests later. Jon has been looking at them already.

I'm not familiar with promises, but I will have a look now and see if I can make it work that way.

[jpw]

Joey you are correct, but 😁

• runPa11yOnTasks is exported by this module so we were not keen on changing what it returns, as we don't know what other code uses it as yet (without more refactoring).
• the try/catch should never catch as long as runPa11yOnTasks doesn't change. This is "illogical" or belt & braces, depending on ones opinion. It also reproduces the original behaviour of the code IIRC which we didn't want to change as this isn't adequately tested yet.

[joeyciechanowicz]

Yeah, fair enough. I've created #94 to add osme unit tests around this so we have a bit of stability.

[josebolos]

Thanks everyone! Can we merge then?