eng: add support for snapshot tests #190444

connor4312 · 2023-08-14T19:44:09Z

This adds Jest-like support for snapshot testing.
Developers can do something like:

await assertSnapshot(myComplexObject)

The first time this is run, the snapshot expectation file is written
to a __snapshots__ directory beside the test file. Subsequent runs
will compare the object to the snapshot, and fail if it doesn't match.

You can see an example of this in the test for snapshots themselves!

After a successful run, any unused snapshots are cleaned up. On a failed
run, a gitignored .actual snapshot file is created beside the
snapshot for easy processing and inspection.

Shortly I will do some integration with the selfhost test extension to
allow developers to easily update snapshots from the vscode UI.

For #189680

cc @ulugbekna @hediet

connor4312 · 2023-08-14T19:46:40Z

test/unit/browser/renderer.html

@@ -116,15 +116,22 @@
 			runner.on('pending', test => window.mocha_report('pending', serializeRunnable(test)));
 		};

+
+		async function loadModules(modules) {


Changing how we load files was necessary for Mocha to populate the currentTest.file, which we need to know where to place snapshots

@ulugbekna

This adds Jest-like support for snapshot testing. Developers can do something like: ```js await assertSnapshot(myComplexObject) ``` The first time this is run, the snapshot expectation file is written to a `__snapshots__` directory beside the test file. Subsequent runs will compare the object to the snapshot, and fail if it doesn't match. You can see an example of this in the test for snapshots themselves! After a successful run, any unused snapshots are cleaned up. On a failed run, a gitignored `.actual` snapshot file is created beside the snapshot for easy processing and inspection. Shortly I will do some integration with the selfhost test extension to allow developers to easily update snapshots from the vscode UI. For #189680 cc @ulugbekna @hediet

ulugbekna

This is super cool, excited to see how this looks in UI!

A couple of questions:

Anything I can help with?
Do you think we could also support inline snapshots?
Can a snapshot be a multi-line string? Is it deindented?
Are we going to support updating snapshots on file/suite/test/assertion levels?

(nice touch with snapception :))

connor4312 · 2023-08-14T21:03:26Z

It looks like merge editor tests started failing in the same way as #173056 😕

connor4312 · 2023-08-14T21:29:15Z

Tried tossing some fixes at components I saw in the stacktraces but I doubt they're the culprit. Every test I've added the disposable tracker to has had leaks. @hediet what do you want to do here?

Re @ulugbekna: could help with current test failures if you wanna be really ambitious 🙃 But for other things, inline snapshots could be cool. For updating all snapshots in a test run, that could be useful as well. Not sure the best UX for that. Multiline strings work -- if the string is the top-level item provided to assertSnapshot, it'll be printed verbatim, otherwise it'll be JSON-encoded.

connor4312 · 2023-08-14T22:14:20Z

I started going up the tests happening before the merge editor tests. Inline chat has lots of undisposed disposables--both in tests and real code. I spent an hour or so just getting fixes for those in, but that's not really sustainable to do everywhere (also wildly out of scope for this PR.) I also don't think we should block doing improvements in test infrastructure (this and #173056) behind it.

In this PR, I will comment out the problematic line. I think fixing it and re-inserting it is good, but I don't want to say we can't have snapshot testing until some indeterminable leak gets fixed...

Fixes some leaks I found while looking at #190444, by adding ensureNoDisposablesAreLeakedInTestSuite to the suite. I don't have a dev setup for inline chat, so I have not tested this beyond running tests and verifying the fix

hediet · 2023-08-15T08:52:38Z

For the diff editor tests, I do this: https://github.com/microsoft/vscode/tree/cb94a70e1f43b4115e86907165c80d7870f10e18/src/vs/editor/test/node/diffing

That way, running the tests already updates the snapshots and you can use the diff editor from the SCM view to review the changes (for the diff algorithm snapshots I have a custom editor that renders the snapshotted diff visually).
To accept the changes, you just delete the *.invalid files.

This works very well in practice.

Also, it could be useful if the test is in control of the file extension of the snapshot - this would enable custom editors to render the snapshot.

hediet · 2023-08-15T09:02:33Z

In this PR, I will comment out the problematic line. I think fixing it and re-inserting it is good, but I don't want to say we can't have snapshot testing until some indeterminable leak gets fixed...

I kind of agree, but with this change, you basically made the indeterminable leak my problem now (which I already investigated for a couple of hours)... Maybe we should come up with a plan for the team to tackle this together.

M4ximumPizza

Looks super cool. I am excited to see how this turns out in the end.

connor4312 · 2023-08-15T15:00:30Z

I agree, I will make an adoption item to use the leak detector in tests people own. Edit: now made in #190503

I'll add an option to customize the extension.

the expected file is updated automatically

This is an interesting approach. Personally I think I would prefer immediate failures in the event that I don't notice an update and end up accidentally committing the "invalid" file and having to wait to PR to get the failure. I think that with extension adoption of #190298 in the selfhost test extension, updating the snapshot should be low enough friction.

hediet · 2023-08-16T09:09:41Z

Personally I think I would prefer immediate failures in the event that I don't notice an update and end up accidentally committing the "invalid" file

Even in my approach you'd get an immediate failure (and that failure is even idempotent because of the invalid files).
When you revert the logic changes, the snapshot test realizes that the actual output matches the invalid file content and then restores the expected file and deletes the invalid file.

The advantage is that you don't need to rerun tests to update the expected files and you also don't have to figure out how to run the update command.

…ionService (#190623) * inline chat: fix leaks found in testing Fixes some leaks I found while looking at #190444, by adding ensureNoDisposablesAreLeakedInTestSuite to the suite. I don't have a dev setup for inline chat, so I have not tested this beyond running tests and verifying the fix * eng: cleanup some leaks around around editors and workbenchInstantiationService I added a disposable leak tracker to a test that used `workbenchInstantiationService`. This fixes the baseline leaks and some extra leaks with that function. * fix build * rm too-eager leak checker * remove forgotten busy loop

…ionService (microsoft#190623) * inline chat: fix leaks found in testing Fixes some leaks I found while looking at microsoft#190444, by adding ensureNoDisposablesAreLeakedInTestSuite to the suite. I don't have a dev setup for inline chat, so I have not tested this beyond running tests and verifying the fix * eng: cleanup some leaks around around editors and workbenchInstantiationService I added a disposable leak tracker to a test that used `workbenchInstantiationService`. This fixes the baseline leaks and some extra leaks with that function. * fix build * rm too-eager leak checker * remove forgotten busy loop

connor4312 enabled auto-merge (squash) August 14, 2023 19:44

connor4312 self-assigned this Aug 14, 2023

vscodenpa added this to the August 2023 milestone Aug 14, 2023

connor4312 commented Aug 14, 2023

View reviewed changes

connor4312 force-pushed the connor4312/snapshot-testing branch from c7330fd to 717e325 Compare August 14, 2023 19:59

connor4312 force-pushed the connor4312/snapshot-testing branch from 717e325 to 352cddf Compare August 14, 2023 20:23

ulugbekna previously approved these changes Aug 14, 2023

View reviewed changes

fix async stacktraces getting clobbered

10b297f

connor4312 dismissed ulugbekna’s stale review via 10b297f August 14, 2023 21:03

random fixes

3d10cc0

comment out leak detector, for now

c95ecb8

connor4312 mentioned this pull request Aug 14, 2023

inline chat: fix leaks found in testing #190455

Merged

M4ximumPizza reviewed Aug 15, 2023

View reviewed changes

add option to snapshot file extension

aefef3a

connor4312 mentioned this pull request Aug 15, 2023

Adopt ensureNoDisposablesAreLeakedInTestSuite in unit tests #190503

Closed

joyceerhl approved these changes Aug 15, 2023

View reviewed changes

connor4312 merged commit 6a847ba into main Aug 15, 2023

connor4312 deleted the connor4312/snapshot-testing branch August 15, 2023 19:03

alexr00 approved these changes Aug 15, 2023

View reviewed changes

github-actions bot locked and limited conversation to collaborators Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eng: add support for snapshot tests #190444

eng: add support for snapshot tests #190444

connor4312 commented Aug 14, 2023

connor4312 Aug 14, 2023

ulugbekna left a comment

connor4312 commented Aug 14, 2023 •

edited

Loading

connor4312 commented Aug 14, 2023 •

edited

Loading

connor4312 commented Aug 14, 2023 •

edited

Loading

hediet commented Aug 15, 2023 •

edited

Loading

hediet commented Aug 15, 2023

M4ximumPizza left a comment

connor4312 commented Aug 15, 2023 •

edited

Loading

hediet commented Aug 16, 2023 •

edited

Loading

eng: add support for snapshot tests #190444

eng: add support for snapshot tests #190444

Conversation

connor4312 commented Aug 14, 2023

connor4312 Aug 14, 2023

Choose a reason for hiding this comment

ulugbekna left a comment

Choose a reason for hiding this comment

connor4312 commented Aug 14, 2023 • edited Loading

connor4312 commented Aug 14, 2023 • edited Loading

connor4312 commented Aug 14, 2023 • edited Loading

hediet commented Aug 15, 2023 • edited Loading

hediet commented Aug 15, 2023

M4ximumPizza left a comment

Choose a reason for hiding this comment

connor4312 commented Aug 15, 2023 • edited Loading

hediet commented Aug 16, 2023 • edited Loading

connor4312 commented Aug 14, 2023 •

edited

Loading

connor4312 commented Aug 14, 2023 •

edited

Loading

connor4312 commented Aug 14, 2023 •

edited

Loading

hediet commented Aug 15, 2023 •

edited

Loading

connor4312 commented Aug 15, 2023 •

edited

Loading

hediet commented Aug 16, 2023 •

edited

Loading