eclipse-theia / theia

Eclipse Theia is a cloud & desktop IDE framework implemented in TypeScript.
http://theia-ide.org
Eclipse Public License 2.0
20.08k stars 2.5k forks source link

[test] ditch sinon module #5429

Open akosyakov opened 5 years ago

akosyakov commented 5 years ago

Sinon library allows to mock JS objects. I see a lot of tests misusing it by mocking internals and relying on timing and execution order of internals. Such tests hard to maintain since they don't allow to change implementations easily. Instead of developers should rely on public APIs and don't make assumptions about internal flow. The issue with Sinon that it provides capabilities to mock such things easily.

I suggest we remove it and use plain JS for object mocking, i.e. <MyService>{}. It will make hard to mock internal control flow and discourage it. Plus we will reduce our dep tree and force to review test for such misusage.

vince-fugnitto commented 5 years ago

I'm fine with removing sinon :)

akosyakov commented 5 years ago

It was pointed out at the dev meeting that we should define what is good test and look at it during PR review. Using sinon itself is not the reason for unstable tests.

I will document definition of good and bad tests with examples here: https://github.com/theia-ide/theia/blob/master/doc/Testing.md