From fa2790d52a527834591a031e7f19f6e0ca103228 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Fri, 27 Mar 2026 17:40:57 -0700 Subject: [PATCH] Make carve outs in property test more explicit (#52610) Refactors the property test to be explicit about the exceptions to the sidebar's "properties" Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Closes #ISSUE Release Notes: - N/A --- crates/sidebar/src/sidebar_tests.rs | 105 +++++++++++++++++++--------- 1 file changed, 71 insertions(+), 34 deletions(-) diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index 04b2d129e0fadabbb3da07364d31ccbbaa965f35..8170a2956886f1bc0b90acd2f83b5a9ccd2c979b 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/crates/sidebar/src/sidebar_tests.rs @@ -4470,10 +4470,46 @@ mod property_test { anyhow::bail!("sidebar should still have an associated multi-workspace"); }; - let all_workspaces: HashSet = multi_workspace - .read(cx) - .workspaces() + let workspaces = multi_workspace.read(cx).workspaces().to_vec(); + + // For each workspace, collect the set of canonical repo paths + // (original_repo_abs_path) from its root repositories. Two + // workspaces that share a canonical repo path are in the same + // linked-worktree group. + let canonical_repos = |ws: &Entity| -> HashSet { + root_repository_snapshots(ws, cx) + .map(|snapshot| snapshot.original_repo_abs_path.to_path_buf()) + .collect::>() + }; + + // Build a map from canonical repo path → set of workspace + // EntityIds that share that repo. + let mut repo_to_workspaces: HashMap> = HashMap::new(); + for ws in &workspaces { + for repo_path in canonical_repos(ws) { + repo_to_workspaces + .entry(repo_path) + .or_default() + .insert(ws.entity_id()); + } + } + + // A workspace participates in a linked-worktree group when it + // shares a canonical repo path with at least one other workspace. + let in_linked_worktree_group = |ws: &Entity| -> bool { + canonical_repos(ws).iter().any(|repo_path| { + repo_to_workspaces + .get(repo_path) + .is_some_and(|members| members.len() > 1) + }) + }; + + // TODO + // Carve-out 1: workspaces with no root paths are not shown + // because the sidebar skips empty path lists. + let expected_workspaces: HashSet = workspaces .iter() + .filter(|ws| !workspace_path_list(ws, cx).paths().is_empty()) .map(|ws| ws.entity_id()) .collect(); @@ -4484,38 +4520,34 @@ mod property_test { .filter_map(|entry| entry.workspace().map(|ws| ws.entity_id())) .collect(); - let stray = &sidebar_workspaces - &all_workspaces; - anyhow::ensure!( - stray.is_empty(), - "sidebar references workspaces not in multi-workspace: {:?}", - stray, - ); - - let workspaces = multi_workspace.read(cx).workspaces().to_vec(); - - // A workspace may not appear directly in entries if another - // workspace in the same group is the representative. Check that - // every workspace is covered by a group that has at least one - // workspace visible in the sidebar entries. - let project_groups = ProjectGroupBuilder::from_multiworkspace(multi_workspace.read(cx), cx); - for ws in &workspaces { - if sidebar_workspaces.contains(&ws.entity_id()) { - continue; - } - let group_has_visible_member = project_groups.groups().any(|(_, group)| { - group.workspaces.contains(ws) - && group - .workspaces - .iter() - .any(|gws| sidebar_workspaces.contains(&gws.entity_id())) - }); + // Check every mismatch between the two sets. Each one must be + // explainable by a known carve-out. + let missing = &expected_workspaces - &sidebar_workspaces; + let stray = &sidebar_workspaces - &expected_workspaces; + + for entity_id in missing.iter().chain(stray.iter()) { + let Some(workspace) = workspaces.iter().find(|ws| ws.entity_id() == *entity_id) else { + anyhow::bail!("workspace {entity_id:?} not found in multi-workspace"); + }; + + // TODO + // Carve-out 2: when multiple workspaces share a linked- + // worktree group, only one representative is shown. Either + // side of the relationship (parent or linked worktree) may + // be the representative, so both can appear in the diff. anyhow::ensure!( - group_has_visible_member, - "workspace {:?} (paths {:?}) is not in sidebar entries and no group member is visible", - ws.entity_id(), - workspace_path_list(ws, cx).paths(), + in_linked_worktree_group(workspace), + "workspace {:?} (paths {:?}) is in the mismatch but does not \ + participate in a linked-worktree group.\n\ + Only in sidebar (stray): {:?}\n\ + Only in multi-workspace (missing): {:?}", + entity_id, + workspace_path_list(workspace, cx).paths(), + stray, + missing, ); } + Ok(()) } @@ -4583,6 +4615,7 @@ mod property_test { .and_then(|cv| cv.read(cx).parent_id(cx)) }); + // TODO: Remove this state entirely anyhow::ensure!( sidebar.agent_panel_visible == panel_actually_visible, "sidebar.agent_panel_visible ({}) does not match AgentPanel::is_visible ({})", @@ -4590,7 +4623,11 @@ mod property_test { panel_actually_visible, ); - // TODO: tighten this once focused_thread tracking is fixed + // TODO: Remove these checks, the focused_thread should _always_ be Some(item-in-the-list) after + // update_entries. if the activated workspace's agent panel has an active thread, this item should + // match the one in the list. There may be a slight delay, where a thread is loading so the agent panel + // returns None initially, and the focused_thread is often optimistically set to the thread the agent panel + // is going to be if sidebar.agent_panel_visible && !sidebar.active_thread_is_draft { if let Some(panel_session_id) = panel_active_session_id { anyhow::ensure!( @@ -4606,7 +4643,7 @@ mod property_test { #[gpui::property_test] async fn test_sidebar_invariants( - #[strategy = gpui::proptest::collection::vec(0u32..DISTRIBUTION_SLOTS * 10, 1..20)] + #[strategy = gpui::proptest::collection::vec(0u32..DISTRIBUTION_SLOTS * 10, 1..5)] raw_operations: Vec, cx: &mut TestAppContext, ) {