Revert merge pull request #20429 (removing chai)

[weswigham]

Chai removing the namespace from their types was a bug, which has been fixed - the correct fix from us would have been pinning our version until a fix was out, but instead we went way overboard and removed the dependency, and I'll be honest, while the assertions are no different when all tests are passing, that's not what there're there for. Failure messages like this one:

Are straight bad compared to what chai gives (a useful comparison of both inputs to equals). Object diffs for deep equals are also much worse (or rather, non-extant). We didn't use an assertion library for no reason, we used it to be more productive when debugging. I don't want to need to open a debugger just to see every small discrepancy! And while I could spend hours of effort making our custom assertion library become up-to-par with what I'd expect for a decent debugging experience (and I know @rbuckton already has his own assertion framework, but he's not ready AFAIK), it's much easy to not have a NIH mindset and continue using the tested and featureful assertion library we've been using for years. Especially since there's not a single issue with it.

[ghost]

assert.equal should already be printing the arguments on failure. Could you test against the latest master branch?

[ghost]

Actually, I see that the current version uses a custom message or prints the arguments, while we could be printing both. I'll make a PR.

[weswigham]

@Andy-MS My point still stands - there's no issue with chai, and it's way better at this. We shouldn't be investing time in this.

[weswigham]

@Andy-MS What you have in master is still really bad compared to chai - chai actually does a git diff-style diff on strings when they don't match, which we sometimes rely heavily on for actually identifying where differences are (ie, in any large fourslash failure).

[weswigham]

@Andy-MS If you have a hankering to design a perfect assertion framework from the ground up, TS-first, I advise collaborating with @rbuckton. He's actually got way more effort than this invested already.

[ghost]

We don't need a framework. We need 8 functions. And I'm not so sure about isFalse since assert(!x) is easier to read. I spend a lot of time writing fourslash tests so if I see any problems I'll fix them.

[weswigham]

@Andy-MS We don't even need 8 functions. But the point is, the framework has a much better developer experience, and is why we use it. And playing catchup with these "8 functions" to get as usable is a waste of time when there's no issues with the framework we were using.

[weswigham]

And I'm not so sure about isFalse since assert(!x) is easier to read.

And is meaningfully different! !"" is true! !0 is true! !NaN is true! As I said! These are rigorous test functions! They should not be so loose - you'd call an isFalsy or a simple assert if you wanted to be loose, not a function named isFalse!

[ghost]

I didn't create that pull request because I want to re-engineer an entire framework -- it was because I thought it was easier to provide a few simple functions than to pull in a framework with much more functionality than needed. (For example, the expect(x).to.be(y) test style was only used in one file, for no apparent reason.) Please don't assume that I'm actively trying to make your life worse.
If you're having problems with the assertion functions, please make issues (plural) for them and I'll fix them. I don't know yet what you mean by it having a better developer experience, but I assume that if a function can have the same outputs for the same inputs that chai has, the developer experience is just as good. I'm happy to waste that time for you if it means having simpler test code.
As always, let's try to follow the spirit of the code of conduct.

[weswigham]

I didn't create that pull request because I want to re-engineer an entire framework -- it was because I thought it was easier to provide a few simple functions than to pull in a framework with much more functionality than needed.

But it's not. Pulling in a framework forces us to do one thing: monitor our dependency version. "Providing a few simple functions" requires testing and verifying our own assertion functions and ensuring that they can provide all the development aids we'd like.

(For example, the expect(x).to.be(y) test style was only used in one file, for no apparent reason.)

It was available as an option, and two years ago one of my first contributions included that test file nobody complained that it differed from the prevailing assertion style and it was merged (at the time, I wasn't even aware if there was a prevailing assertion style). IMO, I actually like expect-style assertions more, but I defer to the style of the file I'm editing.

If you're having problems with the assertion functions, please make issues (plural) for them and I'll fix them. I don't know yet what you mean by it having a better developer experience, but I assume that if a function can have the same outputs for the same inputs that chai has, the developer experience is just as good.

Which is just reimplementing the framework we were using. You've just said "if you miss any features, I'll reimplement them". One of my complaints is that I no longer get string-diffs for long strings. (like colored lines with pluses and minuses). Same for object diffs. You shouldn't go implement a diff algorithm to support that - this is our test subsystem, and that algorithm is going to be pretty large. We should just be leveraging existing code. And the reason I keep repeating that is because removing the assertion framework and replacing it with our own stinks of NIH syndrome. I'm not unfamiliar with replacing parts of it which underperform, but I strongly believe that ripping out all of it was premature, because there are a host of small niceties that nobody fully thought through that we're just throwing away. Removing chai in full didn't make the tests finish 40% faster or anything; there's no tangible benefit - we're taking on the maintenance burden of these assertions for no great reason.

[ghost]

I'm pretty confident that I can satisfy your use-cases without installing or implementing any frameworks. If you can elucidate them I'm happy to make a pull request. I'm not against importing things, so if that involves pulling in a diffing package, that's fine.

[weswigham]

@Andy-MS I looked into it a bit, and it looks like mocha implements diff support for assertion libraries, provided their errors are the appropriate structure (mocha also expects actual and expected properties on the error, which this implementation provides via the _props parameter). This means that minimally we should probably be using the standard structure for an assertion exception instead of a simple throw new Error to leverage that.

But I'm still not sure why we've added ~100 lines (and still growing) of self-supported, mostly undocumented, untested assertion code when we are aware of and have used libraries that already do this (that have dedicated tests and available documentation). Sure they do more, but the more isn't really hurting us. And they have the advantage of having been used for years and been rigorously tested for edge cases because of it. You also have to remember that the more just-for-us our test harness becomes, the harder it becomes for any community contributors to interact with. An advantage to using both mocha and chai was that both see wide use in the greater JS community, so many devs are expected to already have some experience with them, and be familiar with their APIs.

[RyanCavanaugh]

We've been using chai since 1.1 and probably earlier without issue. While it was important to get the CI off the floor (which at the time I believe was trickier than just a lockfile due to some caches), I don't think a one-time break from @types means we should abandon any given widely-used library and start rewriting a portion of it ourselves.

If you're having problems with the assertion functions, please make issues (plural) for them and I'll fix them.
I'm happy to waste that time for you if it means having simpler test code.

Your activity does not exist in a bubble when you work in a team. When you make these PRs, you're not just incurring the cost of your own time, you're incurring cost on the entire team to review these PRs, review the associated activity on the issue tracker, deal with possible merge conflicts, figure out that they have to file issues in the first place, and other costs. The TypeScript repo isn't the place to rearrange the kitchen every few weeks to make it look nicer - other people are counting on the knives being in the third drawer and any change has external costs that should be justified by a manifest upside.

In return what are we getting? The wider surface area of Chai has historically not been a problem, nor has its install size or performance. I don't see what's "simpler" about this either. If I ever have questions about what the assert functions do (deep vs shallow, object identity, NaN), I can read the Chai docs and get answers; instead now I'm reading the source code and hoping those have the same answers. While we were only using a handful of the available asserts in Chai, I think in practice we could be using a lot more and getting better test failure messages.

Worse, we're exposed to a new kind of risk. What happens if one of these assert functions has a bug in it and we ship some bad bug as a result? I've been in the position of having to explain to people in Microsoft that, due to a bug, a framework said their tests were passing when they possibly weren't - it is not a comfortable place to be. If Doug asks "Why did we ship this regression? Because we decided to re-implement the most popular assertion JS library?" there should be a really good answer.

I just don't see the upside of not taking this PR given that we know we still have a nonzero gap between what's present and what's hard-trod available functionality in Chai.

[weswigham]

@RyanCavanaugh

(which at the time I believe was trickier than just a lockfile due to some caches)

Oh! For future reference, (since with dependency caching this could always happen again with any dependency) you can manually purge caches in travis, once you know a later version is out with a fix (or after you've pinned our version to an older one):

It's a somewhat out-of-the-way-place, but it's pretty useful. I do wish I knew how to do it for dotnetci, but I also don't think I've ever witnessed it perform cacheing/any cache issues.

[ghost]

I didn't mean to imply that I didn't consider the error messages to be a real problem -- I do.
I'll merge this and make a followup PR to implement the deepEqual fix from #20429.