ava icon indicating copy to clipboard operation
ava copied to clipboard

First parameter for "snapshot" assertion should be called "value" for consistency

Open DavidAnson opened this issue 2 years ago • 9 comments

Currently, it is called "expected": https://github.com/avajs/ava/blob/main/docs/03-assertions.md#snapshotexpected-message

However, that is inconsistent with other assertion APIs like "is" where parameters are named "value" and "expected": https://github.com/avajs/ava/blob/main/docs/03-assertions.md#isvalue-expected-message

In the case of "snapshot", the expected data is represented by the previously-captured snapshot and the test data (often called "actual") is what gets passed to the function via the first parameter.

DavidAnson avatar Jan 05 '22 01:01 DavidAnson

Sure, could change that. To value? There's the documentation and the TypeScript definition, and then the implementation itself.

novemberborn avatar Jan 08 '22 13:01 novemberborn

I prefer "actual "and "expected" as this project uses in the TypeScript declaration file: https://github.com/avajs/ava/blob/6b63b1bc14a1aaa7d4ee4f2201f901624419c677/types/assertions.d.ts#L184

However, this project seems to use the terms "value" and "expected" in the documentation as I link to here. That is an inconsistency which should probably also be addressed. If you need me to open another issue for that, I can.

However using the name "expected" with the "snapshot" assertion is backwards and misleading and therefore seems more urgent.

DavidAnson avatar Jan 08 '22 18:01 DavidAnson

Here's an issue for the documentation vs. declaration mismatch: https://github.com/avajs/ava/issues/2938

DavidAnson avatar Jan 09 '22 00:01 DavidAnson

Sounds good to me. Would you like to open a PR @DavidAnson?

novemberborn avatar Jan 09 '22 19:01 novemberborn

Please don't wait for me to send a PR, it's unlikely I'll have time for this soon.

DavidAnson avatar Jan 09 '22 19:01 DavidAnson

Looks like snapshot() is fundamentally different from other assertions and thus its currently document param name is correct.

live627 avatar Jan 22 '22 08:01 live627

Please correct me if I'm wrong: When running tests (common scenario), the input to the snapshot function is the actual value observed when running the test. The snapshot function then reads the snapshot file to determine the expected value for that test and diffs the two values.

The situation is different when running in --update-snapshots mode to capture a new snapshot, but that should not be the focus of the API naming, in my opinion.

DavidAnson avatar Jan 22 '22 13:01 DavidAnson

I got that one backwards. I also don't use snapshots.

live627 avatar Jan 23 '22 05:01 live627

Looking at this now I agree actual makes more sense. But I need to have a closer look, see how the diffs are presented and so forth. Simply renaming may not add clarity.

novemberborn avatar Jan 23 '22 18:01 novemberborn