search

microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.99k stars 29.53k forks source link

debugDescriptionSymbol should not be used for snapshots #220048

Open hediet opened 5 months ago

hediet commented 5 months ago

formatValue is used to create snapshots of some values, but is uses debugDescriptionSymbol, which I don't think it should:

https://github.com/microsoft/vscode/blob/76837b663030640b32e6604a4a330411121a4cc3/src/vs/base/test/common/snapshot.ts#L123-L125

In particular, this test :

https://github.com/microsoft/vscode/blob/76837b663030640b32e6604a4a330411121a4cc3/src/vs/workbench/contrib/chat/test/common/chatModel.test.ts#L177-L183

... seems to call this code and includes the result in a snapshot:

https://github.com/microsoft/vscode/blob/76837b663030640b32e6604a4a330411121a4cc3/src/vs/base/common/uri.ts#L417-L419

The problem is that a utility to help debugging is now used to assert that some model logic behaves correctly. That makes tweaks to the utility difficult. The debug description should only be consumed by humans.

(This broke in https://github.com/microsoft/vscode/pull/216537)

connor4312 commented 5 months ago

Snapshots are also verified by humans. Especially for things like ranges, positions, and URIs, the debug description contains the entire contents of the object and it's much easier to verify them than looking at property internals.

If we remove them and always represent the object naively, you get into a similar issue that changes to and implementation details in object internals will also cause tests to fail (like https://github.com/microsoft/vscode/issues/174680). So, in either case you're in a situation that refactors such as #216537 can cause tests to fail.

hediet commented 4 months ago

Especially for things like ranges, positions, and URIs, the debug description contains the entire contents of the object and it's much easier to verify them than looking at property internals.

I understand. In this case, I suggest to have a seperate helper function and to duplicate the debug.description code. I really do believe that changing debug.description should not break anyone, including snapshot tests. Otherwise, the whole promise of the symbol and its advantage over just implementing .toString is gone.