Removing `chai-as-promised` uses #1505

paul-marechal · 2018-03-14T14:20:43Z

This commit fixes issue #928.

Replaced most calls to eventually and should by async/await constructs.

Added @theia/core/lib/common/test/test.js module that exposes a throwsAsync function, that is just an utility to test for Promise rejections.

paul-marechal · 2018-03-14T14:23:20Z

Although, the prototype for test.throwsAsync is based on passing every parameters after the first one to chai.expect(...).throws(params), which could be a bit better than using the current ...args: any[]. I wasn't able to find how to properly define the prototype.

epatpol

Don't forget to remove the dependency to chai-as-promised from the appropriate package.json and update the yarn.lock file

Also it would seem you have to manually edit one of your commit's author as even though you signed off with @ericsson.com your old email info is still in that commit.

epatpol · 2018-03-14T15:33:21Z

packages/core/src/common/test/test.ts

@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2017 Ericsson and others.


Please update to 2018

epatpol · 2018-03-14T15:37:35Z

packages/task/src/node/task-server.slow-spec.ts

 import { testContainer } from './test-resources/inversify.spec-config';
 import { BackendApplication } from '@theia/core/lib/node/backend-application';
 import { TaskExitedEvent, TaskInfo, TaskServer, TaskOptions, ProcessType } from '../common/task-protocol';
 import { TaskWatcher } from '../common/task-watcher';
 import * as ws from 'ws';
 import * as http from 'http';
 import * as https from 'https';
+import * as test from '@theia/core/lib/common/test/test';


Wouldn't import { throwsAsync } from ... work here? This way we wouldn't have to call test.throwsAsync() every time.

In the case of using a utility module I have the habit of doing the module.function() so that upon reading it looks explicit. I would pick-import objects, but functions ?

In the same fashion I think it is possible to change this:

// happens a lot import * as chai from 'chai'; const expect = chai.expect // replacement import { expect } from 'chai';

So is there a convention for the importations ?

I don't think there is a written convention at the moment. And yes I believe we could use import { expect } from 'chai' afaik. But usually we would declare something like const expect = chai.expect right after, as to not bother with having to call chai.expect() every time.

We already import functions at several places in our code so I think it's fine to do it this way. To me it's just simpler as a developer and having to call module.function every time seems like noise.

paul-marechal · 2018-03-14T15:43:29Z

Also when looking at the AppVeyor build logs it seems like the tests for @theia/terminal failed.

It also occurred locally, but it would pass the following times (the test fails once, rerun the tests: now its a success).

I fail to understand why.

epatpol · 2018-03-14T15:44:39Z

The AppVeyor builds seem to be unstable and the failure is probably not related to your changes. Still, @kittaakos can you try restarting them? Thanks

kittaakos · 2018-03-14T15:46:37Z

There was a problem with AppVeyor this "afternoon", they have solved it. And I have restarted the build.

kittaakos

It looks good so far. You need to change a bit here and there, but we are getting closer.

If you do not agree with any of my comments, or it just does not work as I imagine, let me know.

kittaakos · 2018-03-14T17:22:18Z

packages/core/src/common/menu.spec.ts

 import * as chai from "chai";
-import * as chaiAsPromised from "chai-as-promised";
+import "mocha";


I think you can remove this.

kittaakos · 2018-03-14T17:22:35Z

packages/core/src/common/menu.spec.ts

 describe('menu-model-registry', () => {

+    before(() => {


This is just noise, you can remove it.

kittaakos · 2018-03-14T17:22:42Z

packages/core/src/common/messaging/proxy-factory.spec.ts

 describe('Proxy-Factory', () => {
+
+    before(() => {


Not needed.

kittaakos · 2018-03-14T17:22:53Z

packages/core/src/common/selection-service.spec.ts

 describe('selection-service', () => {

+    before(() => {


Please remove.

kittaakos · 2018-03-14T17:23:09Z

packages/core/src/common/selection-service.spec.ts

 import * as chai from "chai";
-import * as chaiAsPromised from "chai-as-promised";
+import "mocha";


You can remove this.

kittaakos · 2018-03-14T17:24:09Z

packages/core/src/common/test/test.ts

+
+import { expect } from 'chai';
+
+export async function throwsAsync(promise: Promise<any>, ...args: any[]): Promise<void> {


args should be a RegExp, right?

Please add a better name, for instance expectThrows and it is not necessarily a Promise.

Well, expect(fn).throws(params) has 2 signatures based on the following interface (extracted from chai's index.d.js):

interface Throw { (expected?: string|RegExp, message?: string): Assertion; (constructor: Error|Function, expected?: string|RegExp, message?: string): Assertion; }

chai's .throws can indeed take a RegEx, but it will test the regex against the error message. Which is not what we want if we want to test the type of Error (usually simply Error).
edit: based on my tests, the RegExp would crash using /Error/ when it would work with the plain Error type.

But when trying to implement the overloads to replace the ...args: any[] I couldn't get it to compile.

Secondly this throwsAsync is useful only when monitoring async functions (or regular functions that return a promise, because async functions return a promise too). The StackOverflow version was testing a call on an async function (await fn()) but the problem is that most of Theia's async functions takes parameters. And at the end of the day the thing that is being awaited is a Promise. Maybe there are other awaitable types I don't know of ?

I could rename it to test.expectThrowsAsync ?

kittaakos · 2018-03-14T17:24:44Z

packages/core/src/common/uri.spec.ts

 describe("uri", () => {

+    before(() => {


Please get rid of this. (This is just some leftover from the copied setup.)

kittaakos · 2018-03-14T17:25:20Z

packages/core/src/common/test/test.ts

@@ -0,0 +1,19 @@
+/*


I do not really like the module name.

Can we have expect?

I wasn't fond of it because I didn't find a good way to not conflict with chai.expect in the tests.

expect will create a conflict unless renamed at import time, but then we could just also find it a better name.

assert would have the same problem.

test was the most simple and conflict free I found, but I don't like it much either. Just the best I could find without conflicts.

If you still want to go with expect just explain how you would use it with chai.expect being bound to the expect name.

Usually in the tests:

import * as chai from 'chai'; const expect = chai.expect; const assert = chai.assert; // not used but common practice online

It doesn't really conflict with chai.expect as you usually use it like this import {methodFromExpectModule} from 'path/to/theia/expect' no? assert and test are not really representative of what it does. I'd rather have module names conflict with each other when they do similar things than import something with a name that isn't descriptive enough imo.

Picking functions to import from a package didn't enjoy me much but if it's alright for everyone then I can roll with it ! (I would usually just pick objects/classes and use the package name to access its functions, but I will do as you say).

lmcbout · 2018-03-14T18:42:10Z

Hi Paul, if it just a matter to compile, look at the code in message-service.ts, it has a function with the optional parameter args:any[], ( protected processMessage(type: MessageType, text: string, args?: any[]): Promise<string ) It might help you to see how it is used

________________________________ De : Paul Maréchal <notifications@github.com> Envoyé : 14 mars 2018 14:37:03 À : theia-ide/theia Cc : Subscribed Objet : Re: [theia-ide/theia] Removing `chai-as-promised` uses (#1505) @marechal-p commented on this pull request.

________________________________ In packages/core/src/common/test/test.ts<#1505 (comment)>:

@@ -0,0 +1,19 @@

+/* I wasn't found of it because I didn't find a good way to not conflict with chai.expect in the tests. expect will create a conflict unless renamed at import time, but then we could just also find it a better name. assert would have the same problem test was the most simple and conflict free I found, but I don't like it much either. Just the best I could find without conflicts. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#1505 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAZk6zNz1FWMxJzu2iXi0K19-jc8t3sTks5teWNOgaJpZM4SqhIa>.

paul-marechal · 2018-03-14T18:51:54Z

@lmcbout The problem wasn't so much about how to use the ...args: any[] notation, but rather how to get rid of it ? Marking you on the relevant part.

epatpol · 2018-03-15T14:13:53Z

I believe you could squash some commits together as some of them are simply logical continuation or/and changes resulting from comments of the commits before them.

kittaakos · 2018-03-15T18:03:12Z

packages/core/src/common/test/expect.ts

+
+import { expect } from 'chai';
+
+export async function expectThrowsAsync(promise: Promise<any>, ...args: any[]): Promise<void> {


Are you planning to adopt the API to chai? #1505 (comment)

Good point, as it crossed my mind that this import is a bit specific.

I didn't know how to allow us to use about any testing suite without getting into too much weirdness...

export async function synchronisedErrorThrower(promise: Promise<any>): Promise<Function> { let synchronous = () => { }; try { await promise; } catch (e) { synchronous = () => { throw e; }; } finally { return synchronous; } }

Usage:

const fn = await synchronizedErrorThrower(promise); expect(fn).to.throw(Error);

Would this be better ?

edit: Ok I don't know where I was going with this. Maybe the following ?

export async function asyncErrorCatcher(promise: Promise<any>): Promise<Error | void> { try { await promise; } catch (e) { return e; } }

Usage:

error = await asyncErrorCatcher(promise); expect(error).to.be.instanceof(Error);

What does make the most sense ?

@kittaakos
this would be the last point to figure out I think in order to end this PR ?

I look into your changes right now and try it out.

kittaakos · 2018-03-20T10:36:26Z

Works nicely! Could you please update your new expect module to have better typings?

/*
 * Copyright (C) 2018 Ericsson and others.
 *
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
 */

import { expect } from 'chai';

// tslint:disable-next-line:no-any
export async function expectThrowsAsync(actual: Promise<any>, expected?: string | RegExp, message?: string): Promise<void>;
// tslint:disable-next-line:no-any
export async function expectThrowsAsync(actual: Promise<any>, constructor: Error | Function, expected?: string | RegExp, message?: string): Promise<void>;
// tslint:disable-next-line:no-any
export async function expectThrowsAsync(promise: Promise<any>, ...args: any[]): Promise<void> {
    let synchronous = () => { };
    try {
        await promise;
    } catch (e) {
        synchronous = () => { throw e; };
    } finally {
        expect(synchronous).throw(...args);
    }
}

Currently:

See the difference:

Are you OK with this too, @akosyakov?

kittaakos · 2018-03-20T10:37:16Z

packages/core/src/common/messaging/proxy-factory.spec.ts

@@ -7,21 +7,13 @@

 import "mocha";


You could get rid of this too.

… removing `chai-as-promised` Relative to eclipse-theia#928 Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>

kittaakos

Thanks for cleaning this up! Please merge.

epatpol suggested changes Mar 14, 2018

View reviewed changes

paul-marechal force-pushed the mp/issue-928 branch from d507c12 to f51d5c9 Compare March 14, 2018 17:20

kittaakos reviewed Mar 14, 2018

View reviewed changes

paul-marechal force-pushed the mp/issue-928 branch 4 times, most recently from 51d4840 to 7053253 Compare March 15, 2018 13:59

paul-marechal force-pushed the mp/issue-928 branch from 7053253 to 341201a Compare March 15, 2018 14:50

epatpol approved these changes Mar 15, 2018

View reviewed changes

kittaakos reviewed Mar 15, 2018

View reviewed changes

paul-marechal force-pushed the mp/issue-928 branch from 341201a to 1e32afe Compare March 19, 2018 21:56

kittaakos reviewed Mar 20, 2018

View reviewed changes

Rewriting part of filesystem tests using the async/await notation and…

793aa47

… removing `chai-as-promised` Relative to eclipse-theia#928 Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>

paul-marechal force-pushed the mp/issue-928 branch from 1e32afe to 793aa47 Compare March 20, 2018 15:44

kittaakos self-requested a review March 23, 2018 08:15

kittaakos approved these changes Mar 23, 2018

View reviewed changes

paul-marechal merged commit 7f3d32d into eclipse-theia:master Mar 23, 2018

paul-marechal deleted the mp/issue-928 branch May 18, 2018 14:24

paul-marechal mentioned this pull request Jun 18, 2018

Get rid of chai-as-promised and use async. #928

Closed


		import { expect } from 'chai';

		export async function throwsAsync(promise: Promise<any>, ...args: any[]): Promise<void> {


		import { expect } from 'chai';

		export async function expectThrowsAsync(promise: Promise<any>, ...args: any[]): Promise<void> {

Removing chai-as-promised uses #1505

Removing chai-as-promised uses #1505

Conversation

paul-marechal commented Mar 14, 2018

paul-marechal commented Mar 14, 2018

epatpol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-marechal commented Mar 14, 2018 • edited Loading

epatpol commented Mar 14, 2018

kittaakos commented Mar 14, 2018

kittaakos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-marechal Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-marechal Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmcbout commented Mar 14, 2018 via email

paul-marechal commented Mar 14, 2018

epatpol commented Mar 15, 2018

Choose a reason for hiding this comment

paul-marechal Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos commented Mar 20, 2018

Choose a reason for hiding this comment

kittaakos left a comment

Choose a reason for hiding this comment

Removing `chai-as-promised` uses #1505

Removing `chai-as-promised` uses #1505

paul-marechal commented Mar 14, 2018 •

edited

Loading

paul-marechal Mar 14, 2018 •

edited

Loading

paul-marechal Mar 14, 2018 •

edited

Loading

paul-marechal Mar 15, 2018 •

edited

Loading