Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
DavidAnson opened this issue Jan 5, 2022 · 9 comments

Comments

@DavidAnson
Copy link

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.

@novemberborn
Copy link
Member

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

@DavidAnson
Copy link
Author

I prefer "actual "and "expected" as this project uses in the TypeScript declaration file:

export interface IsAssertion {

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
Copy link
Author

Here's an issue for the documentation vs. declaration mismatch: #2938

@novemberborn
Copy link
Member

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

@DavidAnson
Copy link
Author

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

@live627
Copy link
Contributor

live627 commented Jan 22, 2022

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

@DavidAnson
Copy link
Author

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.

@live627
Copy link
Contributor

live627 commented Jan 23, 2022

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

@novemberborn
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants