hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-23 05:51:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Refactor test cases and improve snapshot handling

Browse source 
- Simplified the formatting of function calls in `encodePatchUpdate.spec.lua` for better readability.
- Added multiple new snapshot files to enhance end-to-end testing for reference path changes, including scenarios for adding, removing, and handling nonexistent attributes.
- Updated test cases in `apply.rs`, `compute.rs`, and `api.rs` to ensure consistent handling of reference properties and improve clarity in property updates.

These changes enhance the maintainability of the test suite and improve the robustness of snapshot testing.
This commit is contained in:
Joe 2026-02-14 11:36:48 -06:00
parent bbd9c0e0fa
commit c181172460
11 changed files with 463 additions and 232 deletions
Download patch file Download diff file Expand all files Collapse all files

83
src/snapshot/tests/apply.rs
View file

@ -128,11 +128,7 @@ fn apply_ref_property_update() {
intern_tree(&tree, &mut redactions);
let root_id = tree.get_root_id();
let children: Vec<Ref> = tree
.get_instance(root_id)
.unwrap()
.children()
.to_vec();
let children: Vec<Ref> = tree.get_instance(root_id).unwrap().children().to_vec();
let child_a_id = children[0];
let child_b_id = children[1];
@ -164,9 +160,10 @@ fn apply_ref_property_update() {
}
#[test]
fn apply_nil_ref_property_update() {
let mut redactions = RedactionMap::default();
fn apply_nil_ref_skips_in_forward_sync() {
// In the forward-sync path, Ref::none() is a sentinel for "unresolved ref"
// and apply_update_child skips it (line 281-282 of patch_apply.rs).
// This test verifies that behavior is preserved.
let mut tree = RojoTree::new(
InstanceSnapshot::new()
.name("ROOT")
@ -175,7 +172,6 @@ fn apply_nil_ref_property_update() {
.name("Model")
.class_name("Model")]),
);
intern_tree(&tree, &mut redactions);
let root_id = tree.get_root_id();
let model_id = tree
@ -186,7 +182,7 @@ fn apply_nil_ref_property_update() {
.copied()
.unwrap();
// First set PrimaryPart to something
// Set PrimaryPart to something
{
let mut model = tree.get_instance_mut(model_id).unwrap();
model
@ -194,7 +190,8 @@ fn apply_nil_ref_property_update() {
.insert(ustr("PrimaryPart"), Variant::Ref(root_id));
}
// Now apply a patch that sets PrimaryPart to nil (Ref::none())
// In forward-sync, Ref::none() means "unresolved" and is skipped.
// The property should remain unchanged.
let patch_set = PatchSet {
updated_instances: vec![PatchUpdate {
id: model_id,
@ -215,8 +212,62 @@ fn apply_nil_ref_property_update() {
let model = tree.get_instance(model_id).unwrap();
let pp = model.properties().get(&ustr("PrimaryPart"));
assert!(
matches!(pp, Some(Variant::Ref(r)) if r.is_none()),
"PrimaryPart should be nil after applying null Ref"
matches!(pp, Some(Variant::Ref(r)) if *r == root_id),
"Forward-sync: PrimaryPart should remain unchanged when Ref::none() is applied"
);
}
#[test]
fn apply_ref_property_removal() {
// In the two-way sync path, nil Refs are converted to property removals
// (None) in handle_api_write before reaching apply_patch_set. This test
// verifies that None correctly removes the property.
let mut tree = RojoTree::new(
InstanceSnapshot::new()
.name("ROOT")
.class_name("DataModel")
.children(vec![InstanceSnapshot::new()
.name("Model")
.class_name("Model")]),
);
let root_id = tree.get_root_id();
let model_id = tree
.get_instance(root_id)
.unwrap()
.children()
.first()
.copied()
.unwrap();
// Set PrimaryPart to something
{
let mut model = tree.get_instance_mut(model_id).unwrap();
model
.properties_mut()
.insert(ustr("PrimaryPart"), Variant::Ref(root_id));
}
// In two-way sync, nil Refs become None (property removal) before
// reaching the patch system. Verify that None actually removes it.
let patch_set = PatchSet {
updated_instances: vec![PatchUpdate {
id: model_id,
changed_name: None,
changed_class_name: None,
changed_properties: UstrMap::from_iter([(ustr("PrimaryPart"), None)]),
changed_metadata: None,
}],
..Default::default()
};
let applied = apply_patch_set(&mut tree, patch_set);
assert_eq!(applied.updated.len(), 1);
let model = tree.get_instance(model_id).unwrap();
assert!(
model.properties().get(&ustr("PrimaryPart")).is_none(),
"PrimaryPart should be removed after applying None (two-way sync nil Ref)"
);
}
@ -234,11 +285,7 @@ fn apply_multiple_ref_property_updates() {
);
let root_id = tree.get_root_id();
let children: Vec<Ref> = tree
.get_instance(root_id)
.unwrap()
.children()
.to_vec();
let children: Vec<Ref> = tree.get_instance(root_id).unwrap().children().to_vec();
let part_a = children[0];
let part_b = children[1];
let model_id = children[2];

52
src/snapshot/tests/compute.rs
View file

@ -178,12 +178,8 @@ fn tree_with_model_and_parts() -> RojoTree {
.name("Model")
.class_name("Model")
.children(vec![
InstanceSnapshot::new()
.name("Target")
.class_name("Part"),
InstanceSnapshot::new()
.name("OtherPart")
.class_name("Part"),
InstanceSnapshot::new().name("Target").class_name("Part"),
InstanceSnapshot::new().name("OtherPart").class_name("Part"),
])])]);
RojoTree::new(snapshot)
}
@ -225,18 +221,16 @@ fn ref_attr_resolves_primary_part() {
)]))
.children(vec![
InstanceSnapshot::new().name("Target").class_name("Part"),
InstanceSnapshot::new()
.name("OtherPart")
.class_name("Part"),
InstanceSnapshot::new().name("OtherPart").class_name("Part"),
]);
let patch_set = compute_patch_set(Some(snapshot), &tree, model_id);
// The patch should contain an update with PrimaryPart as a Variant::Ref
let has_primary_part_ref = patch_set.updated_instances.iter().any(|u| {
u.changed_properties.get(&ustr("PrimaryPart")).map_or(false, |v| {
matches!(v, Some(Variant::Ref(r)) if r.is_some())
})
u.changed_properties
.get(&ustr("PrimaryPart"))
.map_or(false, |v| matches!(v, Some(Variant::Ref(r)) if r.is_some()))
});
assert!(
has_primary_part_ref,
@ -334,9 +328,7 @@ fn ref_attr_multiple_on_same_instance() {
)]))
.children(vec![
InstanceSnapshot::new().name("Target").class_name("Part"),
InstanceSnapshot::new()
.name("OtherPart")
.class_name("Part"),
InstanceSnapshot::new().name("OtherPart").class_name("Part"),
]);
let patch_set = compute_patch_set(Some(snapshot), &tree, model_id);
@ -362,7 +354,7 @@ fn ref_attr_multiple_on_same_instance() {
}
#[test]
fn ref_attr_nonexistent_path_resolves_to_none() {
fn ref_attr_nonexistent_path_does_not_produce_valid_ref() {
let tree = tree_with_workspace_part();
let root_id = tree.get_root_id();
let workspace_id = tree
@ -395,15 +387,17 @@ fn ref_attr_nonexistent_path_resolves_to_none() {
let patch_set = compute_patch_set(Some(snapshot), &tree, model_id);
// Non-existent path should resolve to None (nil Ref)
let has_nil_ref = patch_set.updated_instances.iter().any(|u| {
// A non-existent path should NOT produce a valid (non-nil) Ref.
// compute_ref_properties returns None for unresolvable paths, which
// means "property value not set" -- not "set to nil Ref".
let has_valid_ref = patch_set.updated_instances.iter().any(|u| {
u.changed_properties
.get(&ustr("PrimaryPart"))
.map_or(false, |v| matches!(v, Some(Variant::Ref(r)) if r.is_none()) || v.is_none())
.map_or(false, |v| matches!(v, Some(Variant::Ref(r)) if r.is_some()))
});
assert!(
has_nil_ref || patch_set.updated_instances.is_empty(),
"Non-existent path should not produce a valid Ref"
!has_valid_ref,
"Non-existent path should not produce a valid (non-nil) Ref"
);
}
@ -444,9 +438,7 @@ fn ref_attr_with_regular_attributes_mixed() {
)]))
.children(vec![
InstanceSnapshot::new().name("Target").class_name("Part"),
InstanceSnapshot::new()
.name("OtherPart")
.class_name("Part"),
InstanceSnapshot::new().name("OtherPart").class_name("Part"),
]);
let patch_set = compute_patch_set(Some(snapshot), &tree, model_id);
@ -480,10 +472,7 @@ fn ref_attr_empty_path_resolves_to_root() {
);
let mut attrs = Attributes::new();
attrs.insert(
"Rojo_Ref_PrimaryPart".into(),
Variant::String("".into()),
);
attrs.insert("Rojo_Ref_PrimaryPart".into(), Variant::String("".into()));
let snapshot = InstanceSnapshot::new()
.name("Model")
@ -497,9 +486,10 @@ fn ref_attr_empty_path_resolves_to_root() {
// Empty path should resolve to root (RojoTree.get_instance_by_path("") returns root)
let has_ref = patch_set.updated_instances.iter().any(|u| {
u.changed_properties
.get(&ustr("PrimaryPart"))
.map_or(false, |v| matches!(v, Some(Variant::Ref(r)) if *r == root_id))
u.changed_properties.get(&ustr("PrimaryPart")).map_or(
false,
|v| matches!(v, Some(Variant::Ref(r)) if *r == root_id),
)
});
assert!(has_ref, "Empty path should resolve to root instance");
}

30
src/web/api.rs
View file

@ -633,12 +633,30 @@ impl ApiService {
let updated_instances = request
.updated
.into_iter()
.map(|update| PatchUpdate {
id: update.id,
changed_class_name: update.changed_class_name,
changed_name: update.changed_name,
changed_properties: update.changed_properties,
changed_metadata: None,
.map(|update| {
// Convert nil Refs (Ref::none()) to property removals (None).
//
// In the two-way sync path, Ref::none() means "user explicitly
// cleared this property" (e.g., set PrimaryPart to nil). But
// apply_update_child skips nil Refs because in the forward-sync
// path, Ref::none() is a sentinel for "unresolved ref."
//
// Converting to None ensures the property is actually removed
// from the tree, keeping it consistent with the filesystem
// (where the Rojo_Ref_* attribute was already removed).
let mut changed_properties = update.changed_properties;
for (_key, value) in changed_properties.iter_mut() {
if matches!(value, Some(Variant::Ref(r)) if r.is_none()) {
*value = None;
}
}
PatchUpdate {
id: update.id,
changed_class_name: update.changed_class_name,
changed_name: update.changed_name,
changed_properties,
changed_metadata: None,
}
})
.collect();