Theia: Get rid of `chai-as-promised` and use `async`.

[kittaakos]

One good place to start with the clean-up is the FS tests.

We could get rid of the before in the tests, and simply use await in the tests.

[marechal-p]

I have been trying to get my head around this issue, and wondered what was your point of view today ?

I rewrote most of the tests for @theia/filesystem: commit.

But I can't see how to test for Promise rejection without using the should statement provided by chai-as-promised.

What would you reckon ?

[kittaakos]

@marechal-p, good point. Would you be fine with something like this?

import { expect } from 'chai';

describe('testMe', async () => {

    it('resolve', async () => {
        expect(await testMe(true)).to.be.true;
    });

    it('reject', async () => {
        try {
            await testMe(false);
            throw new Error('Expected a rejection.');
        } catch (e) {
            expect(e.message).to.be.equal('arg was false');
        }
    });

});

function testMe(arg: boolean): Promise<boolean> {
    return new Promise<boolean>((resolve, reject) => {
        if (!arg) {
            return reject(new Error('arg was false'));
        }
        resolve(arg);
    });
}

[marechal-p]

Throwing an error directly inside the try statement could do the trick, though I'm not fond of testing the exact string of an error...

Maybe something along those lines:

import { expect } from 'chai';

// unique error thrown from tests
class TestError extends Error {}

describe('testMe', async () => {

    it('resolve', async () => {
        expect(await testMe(true)).to.be.true;
    });

    it('reject', async () => {
        try {
            await testMe(false);
            throw new TestError('Expected a rejection.');
        } catch (e) {
            expect(e).to.not.be.instanceof(TestError);
        }
    });

});

function testMe(arg: boolean): Promise<boolean> {
    return new Promise<boolean>((resolve, reject) => {
        if (!arg) {
            return reject(new Error('arg was false'));
        }
        resolve(arg);
    });
}

But at this point I'm wondering if trying to get rid of chai-as-promised is really important ?
Because the .should syntax is more in line with the other statements from chai ?

Just want to make sure that this change is necessary.

[kittaakos]

So the initial idea of this task was the following: we had the first tests with chai-as-promised. These tests require a special setup to enable the chai-as-promised extension in chai. Then later, this setup phase was blindly copied to all other tests which either do not need this (because it can be solved with async) or not even testing any asynchronous calls. I am OK if we just adjust the requirements of this task, and clean up the tests and get rid of the unnecessary chai-as-promised setups. But we could also introduce our expectThrows function and use that instead. What's your take on that?

CC: @akosyakov

[akosyakov]

let's clean up first with keeping chai-as-promised where it makes sense? If it is couple places at the end then I will be in favor of dropping it and introducing expectThrows

[marechal-p]

Are you talking about the following piece ?

    before(() => {
        chai.config.showDiff = true;
        chai.config.includeStack = true;
        chai.should();
        chai.use(chaiAsPromised);
    });

Because when looking at http://chaijs.com/guide/plugins/ in order to see how to add assertion statements, it seems like we would need something that resembles it.

import * as chai from "chai";
import * as our_plugin from ".../x/y/plugin";

var expect = chai.expect;

chai.use(our_plugin);

So I don't really know where we would place this expectThrow function. Tried to declare it like so:

    async function expectThrow(promise: Promise<any>, errorType = Error) {
        try {
            await promise;
            throw new TestError('Expected a rejection');
        } catch (e) {
            expect(e).to.not.be.instanceof(TestError);
            expect(e).to.be.instanceof(errorType);
        }
    }

To be used like:

await expectThrow(asyncFunction(), Error);

But the expect statements don't seem to bubble up to the tests (the test pass despite my attempts to make it not pass).

Sorry for getting into so much details on this, just trying to clear things out.

[kittaakos]

Are you talking about the following piece ?

Yes.

Tried to declare it like so:

I like that. I will check what could be the problem right now.

Sorry for getting into so much details on this, just trying to clear things out.

Hey, there is nothing to be sorry about.

I get back to you as soon as possible.

[kittaakos]

You were very very close. The custom error construction is a bit tricky if you would like to use instanceof on that.

Let me know what do you think:

import { expect } from 'chai';

// tslint:disable:no-unused-expression

class UnfulfilledPromiseRejectionError extends Error {
    constructor(message: string = 'Expected a promise rejection.') {
        super(message);
        Object.setPrototypeOf(this, UnfulfilledPromiseRejectionError.prototype);
    }
}

describe('testMe', async () => {

    it('resolve', async () => {
        expect(await testMe(true)).to.be.true;
    });

    it('reject - OK Error', async () => {
        await expectThrows(testMe(false), Error);
    });

    it('reject - OK ReferenceError', async () => {
        await expectThrows(testMe(false), ReferenceError);
    });

    it('reject - NOK was not rejected', async () => {
        await expectThrows(testMe(true), Error);
    });

    it('reject - NOK different error', async () => {
        await expectThrows(testMe(false), EvalError);
    });

});

/** Throws a reference error if the argument is `false`.s */
function testMe(arg: boolean): Promise<boolean> {
    return new Promise<boolean>((resolve, reject) => {
        if (!arg) {
            return reject(new ReferenceError('arg was false'));
        }
        resolve(arg);
    });
}

// tslint:disable-next-line:no-any
export async function expectThrows(promise: Promise<any>, errorType: any) {
    try {
        await promise;
        throw new UnfulfilledPromiseRejectionError();
    } catch (e) {
        if (e instanceof UnfulfilledPromiseRejectionError) {
            throw new Error(`The promise was not rejected.`);
        } else {
            expect(e).to.be.instanceof(errorType, `Expected an error of type ${errorType.name}, got ${e.name} instead.`);
        }
    }
}

[kittaakos]

We could have expectThrows(promise: MaybePromise<any>, errorType: any) instead. So it would work for both synchronous and asynchronous code.

[akosyakov]

fyi: https://stackoverflow.com/questions/35782435/node-js-assert-throws-with-async-functions-promises

[kittaakos]

I am OK with both although I do not see any reason throwing anything but Error.

[marechal-p]

I really like the one linked on StackOverflow:

async function assertThrowsAsync(fn, regExp) {
    let f = () => {};
    try {
        await fn();
    } catch(e) {
        f = () => {throw e};
    } finally {
        assert.throws(f, regExp);
    }
}

Also it seems like only the FS test suite is actually using chai-as-promised, other tests setup the .should() but seem to never use it.

Getting rid of chai-as-promised seems to be relevant, but because we could still test for async exceptions in other files I wonder where to write the expectThrowsAsync ?

Do we place it in such a way that it can be imported from tests that wants it ? If so where ?
Or do we place it right in the file that uses it (/filesystem/src/node/node-filesystem.spec.ts) ?

[kittaakos]

I really like the one linked on StackOverflow:

👍

other tests setup the .should() but seem to never use it.

That was the actual goal of this task. Thanks for checking.

I wonder where to write the expectThrowsAsync ?

What about creating a new module for it under the: /theia/packages/core/src/common/test/expect.ts location?

[marechal-p]

Turns out I was looking for the use of .should, but when looking for chaiAsPromised usage it came up in the following packages:

• process
• task
• terminal

They mostly use the .to.be.eventually.fulfilled notation.

The only requirement is to write chais.use(chaiAsPromise), do you reckon getting rid of this too ?

This is a trivial issue but understanding the logic here may help me understand the general mindset to have later.

[kittaakos]

I do not know this off the top of my head but isn't expect(yourAsyncCall).to.be.eventually.fulfilled equal to await yourAsyncCall in this case? In other words, if you just want to check something is not rejected, you can do an await if it was rejected an error will be thrown anyway. Does this clear or shall we discuss it further?

[marechal-p]

It didn't cross my mind at all. But you are totally right. Going with this :)

[marechal-p]

https://github.com/theia-ide/theia/pull/1505