Skip to content

Commit

Permalink
Add snapshot test for 2 wire objects
Browse files Browse the repository at this point in the history
Summary:
This diff adds a way to do snapshot tests in SC, by simply calling a macro. It uses that as an example on two wire objects to show it works.

Motivation:
- Using snapshot testing in wire object will make it super clear what changes are happening on the wire format, which is useful to prevent breakages. (see quip)
- I'm going with approach #3 from proposal, but both #2 and #3 would need snapshot testing.

How?
- I'm using [insta](https://docs.rs/insta/1.8.0/insta/), a rust crate for snapshot testing. Unfortunately it depends on some cargo-specific stuff that doesn't work with buck, but I was able to get it working without patching the package by bypassing the Cargo dependent stuff.
- Insta also provides some cargo exensions on top of it to compare snapshots, which we can't have, so I made it clear on the test errors (see test plan) how to update the snapshots (via flag).

Future
- On future diffs I'll add a snapshot test for all the wire objects, this just adds to a first few to show it works and set up the plumbing.
- Writing objects manually for snapshot tests for **every** wire object would be a large amount of code, and troublesome when adding new (specially big) wire objects. We already have wire objects implementing `Arbitrary`, so I think it should be possible to use this to generate simple snapshot tests on automatically generated objects. (It will need some extra work to make sure they are created consistently.)

Reviewed By: markbt

Differential Revision: D30958077

fbshipit-source-id: 3d8663e7897e5f6eb4b97c24f47b37ef2f138e5a
  • Loading branch information
yancouto authored and facebook-github-bot committed Oct 1, 2021
1 parent a95661f commit 30cc107
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 1 deletion.
2 changes: 1 addition & 1 deletion eden/scm/lib/edenapi/types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ bytes = { version = "1.0", features = ["serde"] }
dag-types = { path = "../../dag/dag-types", features = ["for-tests"] }
faster-hex = "0.4"
quickcheck = "1.0"
rand = { version = "0.8", features = ["small_rng"] }
revisionstore_types = { path = "../../revisionstore/types" }
serde = { version = "1.0.126", features = ["derive", "rc"] }
serde_bytes = "0.11"
Expand All @@ -20,6 +19,7 @@ thiserror = "1.0.29"
types = { path = "../../types" }

[dev-dependencies]
insta_ext = { path = "../../insta_ext" }
paste = "1.0"
quickcheck_macros = "1.0"
serde_cbor = "0.11"
Expand Down
8 changes: 8 additions & 0 deletions eden/scm/lib/edenapi/types/src/wire/commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -994,6 +994,14 @@ mod tests {

use quickcheck::quickcheck;

#[test]
fn test_snapshot() {
insta_ext::assert_json!(WireSnapshotState {});
insta_ext::assert_json!(WireEphemeralPrepareResponse {
bubble_id: NonZeroU64::new(12)
});
}

quickcheck! {
fn test_roundtrip_serialize_location(v: WireCommitLocation) -> bool {
check_serialize_roundtrip(v)
Expand Down
8 changes: 8 additions & 0 deletions eden/scm/lib/insta_ext/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# @generated by autocargo from //eden/scm/lib/insta_ext:insta_ext
[package]
name = "insta_ext"
version = "0.1.0"
edition = "2018"

[dependencies]
insta = "1"
1 change: 1 addition & 0 deletions eden/scm/lib/insta_ext/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This crate allows using the insta crate, for snapshot tests, without needing to use Cargo.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
source: commit.rs
expression: "WireEphemeralPrepareResponse{bubble_id: NonZeroU64::new(12),}"

---
{"1":12}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
source: commit.rs
expression: "WireSnapshotState{}"

---
{}
66 changes: 66 additions & 0 deletions eden/scm/lib/insta_ext/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This software may be used and distributed according to the terms of the
* GNU General Public License version 2.
*/

pub fn setup() {
const WORKSPACE: &str = "INSTA_WORKSPACE_ROOT";
const UPDATE: &str = "INSTA_UPDATE";
if std::env::var(WORKSPACE).is_err() {
let mut root = std::path::PathBuf::from(file!());
root.pop();
root.pop();
std::env::set_var(WORKSPACE, root);
}
if std::env::var(UPDATE).is_err() {
std::env::set_var(UPDATE, "no");
}
}

pub fn run(is_cargo: bool, snapshot: &str, file: &str, module: &str, line: u32, expr: &str) {
let command = if is_cargo {
"INSTA_UPDATE=1 cargo test ..."
} else {
"buck test ... -- --env INSTA_UPDATE=1"
};
println!(
"{:=^80}\n",
format!(" Run `{}` to update snapshots ", command)
);
let file_name = std::path::Path::new(file)
.file_name()
.and_then(|p| p.to_str())
.unwrap();
insta::_macro_support::assert_snapshot(
insta::_macro_support::AutoName.into(),
snapshot,
"unused",
// buck builds have a _unittest module suffix which cargo doesn't
// this makes the snapshot location consistent on both
&module.replacen("_unittest", "", 1),
file_name,
line,
expr,
)
.unwrap();
}

/// Assert that the serde json representation of given expression matches the snapshot
/// stored on disk.
#[macro_export]
macro_rules! assert_json {
($value: expr) => {{
$crate::setup();

$crate::run(
option_env!("CARGO_MANIFEST_DIR").is_some(),
&serde_json::to_string(&$value).unwrap(),
file!(),
module_path!(),
line!(),
stringify!($value),
);
}};
}

0 comments on commit 30cc107

Please sign in to comment.