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

Resolve tests per file instead of per crate in test explorer #16971

Merged
merged 1 commit into from
Mar 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/ide/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@ impl Analysis {
self.with_db(|db| test_explorer::discover_tests_in_crate(db, crate_id))
}

pub fn discover_tests_in_file(&self, file_id: FileId) -> Cancellable<Vec<TestItem>> {
self.with_db(|db| test_explorer::discover_tests_in_file(db, file_id))
}

/// Renders the crate graph to GraphViz "dot" syntax.
pub fn view_crate_graph(&self, full: bool) -> Cancellable<Result<String, String>> {
self.with_db(|db| view_crate_graph::view_crate_graph(db, full))
Expand Down
68 changes: 62 additions & 6 deletions crates/ide/src/test_explorer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use ide_db::{
};
use syntax::TextRange;

use crate::{navigation_target::ToNav, runnables::runnable_fn, Runnable, TryToNav};
use crate::{runnables::runnable_fn, NavigationTarget, Runnable, TryToNav};

#[derive(Debug)]
pub enum TestItemKind {
Expand Down Expand Up @@ -56,17 +56,22 @@ fn find_crate_by_id(crate_graph: &CrateGraph, crate_id: &str) -> Option<CrateId>
})
}

fn discover_tests_in_module(db: &RootDatabase, module: Module, prefix_id: String) -> Vec<TestItem> {
fn discover_tests_in_module(
db: &RootDatabase,
module: Module,
prefix_id: String,
only_in_this_file: bool,
) -> Vec<TestItem> {
let sema = Semantics::new(db);

let mut r = vec![];
for c in module.children(db) {
let module_name =
c.name(db).as_ref().and_then(|n| n.as_str()).unwrap_or("[mod without name]").to_owned();
let module_id = format!("{prefix_id}::{module_name}");
let module_children = discover_tests_in_module(db, c, module_id.clone());
let module_children = discover_tests_in_module(db, c, module_id.clone(), only_in_this_file);
if !module_children.is_empty() {
let nav = c.to_nav(db).call_site;
let nav = NavigationTarget::from_module_to_decl(sema.db, c).call_site;
r.push(TestItem {
id: module_id,
kind: TestItemKind::Module,
Expand All @@ -76,7 +81,9 @@ fn discover_tests_in_module(db: &RootDatabase, module: Module, prefix_id: String
text_range: Some(nav.focus_or_full_range()),
runnable: None,
});
r.extend(module_children);
if !only_in_this_file || c.is_inline(db) {
r.extend(module_children);
}
}
}
for def in module.declarations(db) {
Expand Down Expand Up @@ -112,6 +119,55 @@ pub(crate) fn discover_tests_in_crate_by_test_id(
discover_tests_in_crate(db, crate_id)
}

pub(crate) fn discover_tests_in_file(db: &RootDatabase, file_id: FileId) -> Vec<TestItem> {
let sema = Semantics::new(db);

let Some(module) = sema.file_to_module_def(file_id) else { return vec![] };
let Some((mut tests, id)) = find_module_id_and_test_parents(&sema, module) else {
return vec![];
};
tests.extend(discover_tests_in_module(db, module, id, true));
tests
}

fn find_module_id_and_test_parents(
sema: &Semantics<'_, RootDatabase>,
module: Module,
) -> Option<(Vec<TestItem>, String)> {
let Some(parent) = module.parent(sema.db) else {
let name = module.krate().display_name(sema.db)?.to_string();
return Some((
vec![TestItem {
id: name.clone(),
kind: TestItemKind::Crate(module.krate().into()),
label: name.clone(),
parent: None,
file: None,
text_range: None,
runnable: None,
}],
name,
));
};
let (mut r, mut id) = find_module_id_and_test_parents(sema, parent)?;
let parent = Some(id.clone());
id += "::";
let module_name = &module.name(sema.db);
let module_name = module_name.as_ref().and_then(|n| n.as_str()).unwrap_or("[mod without name]");
id += module_name;
let nav = NavigationTarget::from_module_to_decl(sema.db, module).call_site;
r.push(TestItem {
id: id.clone(),
kind: TestItemKind::Module,
label: module_name.to_owned(),
parent,
file: Some(nav.file_id),
text_range: Some(nav.focus_or_full_range()),
runnable: None,
});
Some((r, id))
}

pub(crate) fn discover_tests_in_crate(db: &RootDatabase, crate_id: CrateId) -> Vec<TestItem> {
let crate_graph = db.crate_graph();
if !crate_graph[crate_id].origin.is_local() {
Expand All @@ -133,6 +189,6 @@ pub(crate) fn discover_tests_in_crate(db: &RootDatabase, crate_id: CrateId) -> V
text_range: None,
runnable: None,
}];
r.extend(discover_tests_in_module(db, module, crate_test_id));
r.extend(discover_tests_in_module(db, module, crate_test_id, false));
r
}
8 changes: 6 additions & 2 deletions crates/rust-analyzer/src/handlers/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,12 @@ pub(crate) fn handle_discover_test(
let (tests, scope) = match params.test_id {
Some(id) => {
let crate_id = id.split_once("::").map(|it| it.0).unwrap_or(&id);
(snap.analysis.discover_tests_in_crate_by_test_id(crate_id)?, vec![crate_id.to_owned()])
(
snap.analysis.discover_tests_in_crate_by_test_id(crate_id)?,
Some(vec![crate_id.to_owned()]),
)
}
None => (snap.analysis.discover_test_roots()?, vec![]),
None => (snap.analysis.discover_test_roots()?, None),
};
for t in &tests {
hack_recover_crate_name::insert_name(t.id.clone());
Expand All @@ -254,6 +257,7 @@ pub(crate) fn handle_discover_test(
})
.collect(),
scope,
scope_file: None,
})
}

Expand Down
3 changes: 2 additions & 1 deletion crates/rust-analyzer/src/lsp/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ pub struct TestItem {
#[serde(rename_all = "camelCase")]
pub struct DiscoverTestResults {
pub tests: Vec<TestItem>,
pub scope: Vec<String>,
pub scope: Option<Vec<String>>,
pub scope_file: Option<Vec<TextDocumentIdentifier>>,
}

pub enum DiscoverTest {}
Expand Down
25 changes: 11 additions & 14 deletions crates/rust-analyzer/src/main_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ use std::{
use always_assert::always;
use crossbeam_channel::{never, select, Receiver};
use ide_db::base_db::{SourceDatabase, SourceDatabaseExt, VfsPath};
use itertools::Itertools;
use lsp_server::{Connection, Notification, Request};
use lsp_types::notification::Notification as _;
use lsp_types::{notification::Notification as _, TextDocumentIdentifier};
use stdx::thread::ThreadIntent;
use vfs::FileId;

Expand Down Expand Up @@ -533,22 +532,14 @@ impl GlobalState {
let snapshot = self.snapshot();
move || {
let tests = subscriptions
.into_iter()
.filter_map(|f| snapshot.analysis.crates_for(f).ok())
.flatten()
.unique()
.filter_map(|c| snapshot.analysis.discover_tests_in_crate(c).ok())
.iter()
.copied()
.filter_map(|f| snapshot.analysis.discover_tests_in_file(f).ok())
.flatten()
.collect::<Vec<_>>();
for t in &tests {
hack_recover_crate_name::insert_name(t.id.clone());
}
let scope = tests
.iter()
.filter_map(|t| Some(t.id.split_once("::")?.0))
.unique()
.map(|it| it.to_owned())
.collect();
Task::DiscoverTest(lsp_ext::DiscoverTestResults {
tests: tests
.into_iter()
Expand All @@ -557,7 +548,13 @@ impl GlobalState {
to_proto::test_item(&snapshot, t, line_index.as_ref())
})
.collect(),
scope,
scope: None,
scope_file: Some(
subscriptions
.into_iter()
.map(|f| TextDocumentIdentifier { uri: to_proto::url(&snapshot, f) })
.collect(),
),
})
}
});
Expand Down
8 changes: 6 additions & 2 deletions docs/dev/lsp-extensions.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!---
lsp/ext.rs hash: d5febcbf63650753
lsp/ext.rs hash: 223f48a89a5126a0

If you need to change the above hash to make the test pass, please check if you
need to adjust this doc as well and ping this issue:
Expand Down Expand Up @@ -440,7 +440,11 @@ interface DiscoverTestResults {
// For each test which its id is in this list, the response
// contains all tests that are children of this test, and
// client should remove old tests not included in the response.
scope: string[];
scope: string[] | undefined;
// For each file which its uri is in this list, the response
// contains all tests that are located in this file, and
// client should remove old tests not included in the response.
scopeFile: lc.TextDocumentIdentifier[] | undefined;
}
```

Expand Down
6 changes: 5 additions & 1 deletion editors/code/src/lsp_ext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ export type TestItem = {
range?: lc.Range | undefined;
runnable?: Runnable | undefined;
};
export type DiscoverTestResults = { tests: TestItem[]; scope: string[] };
export type DiscoverTestResults = {
tests: TestItem[];
scope: string[] | undefined;
scopeFile: lc.TextDocumentIdentifier[] | undefined;
};
export type TestState =
| { tag: "failed"; message: string }
| { tag: "passed" }
Expand Down
60 changes: 46 additions & 14 deletions editors/code/src/test_explorer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const prepareTestExplorer = (
) => {
let currentTestRun: vscode.TestRun | undefined;
let idToTestMap: Map<string, vscode.TestItem> = new Map();
const fileToTestMap: Map<string, vscode.TestItem[]> = new Map();
const idToRunnableMap: Map<string, ra.Runnable> = new Map();

testController.createRunProfile(
Expand Down Expand Up @@ -59,6 +60,18 @@ export const prepareTestExplorer = (
false,
);

const deleteTest = (item: vscode.TestItem, parentList: vscode.TestItemCollection) => {
parentList.delete(item.id);
idToTestMap.delete(item.id);
idToRunnableMap.delete(item.id);
if (item.uri) {
fileToTestMap.set(
item.uri.toString(),
fileToTestMap.get(item.uri.toString())!.filter((t) => t.id !== item.id),
);
}
};

const addTest = (item: ra.TestItem) => {
const parentList = item.parent
? idToTestMap.get(item.parent)!.children
Expand All @@ -76,7 +89,7 @@ export const prepareTestExplorer = (
oldTest.range = range;
return;
}
parentList.delete(item.id);
deleteTest(oldTest, parentList);
}
const iconToVscodeMap = {
package: "package",
Expand All @@ -91,14 +104,20 @@ export const prepareTestExplorer = (
test.range = range;
test.canResolveChildren = item.canResolveChildren;
idToTestMap.set(item.id, test);
if (uri) {
if (!fileToTestMap.has(uri.toString())) {
fileToTestMap.set(uri.toString(), []);
}
fileToTestMap.get(uri.toString())!.push(test);
}
if (item.runnable) {
idToRunnableMap.set(item.id, item.runnable);
}
parentList.add(test);
};

const addTestGroup = (testsAndScope: ra.DiscoverTestResults) => {
const { tests, scope } = testsAndScope;
const { tests, scope, scopeFile } = testsAndScope;
const testSet: Set<string> = new Set();
for (const test of tests) {
addTest(test);
Expand All @@ -107,25 +126,38 @@ export const prepareTestExplorer = (
// FIXME(hack_recover_crate_name): We eagerly resolve every test if we got a lazy top level response (detected
// by checking that `scope` is empty). This is not a good thing and wastes cpu and memory unnecessarily, so we
// should remove it.
if (scope.length === 0) {
if (!scope && !scopeFile) {
for (const test of tests) {
void testController.resolveHandler!(idToTestMap.get(test.id));
}
}
if (!scope) {
return;
if (scope) {
const recursivelyRemove = (tests: vscode.TestItemCollection) => {
for (const [_, test] of tests) {
if (!testSet.has(test.id)) {
deleteTest(test, tests);
} else {
recursivelyRemove(test.children);
}
}
};
for (const root of scope) {
recursivelyRemove(idToTestMap.get(root)!.children);
}
}
const recursivelyRemove = (tests: vscode.TestItemCollection) => {
for (const [testId, _] of tests) {
if (!testSet.has(testId)) {
tests.delete(testId);
} else {
recursivelyRemove(tests.get(testId)!.children);
if (scopeFile) {
const removeByFile = (file: vscode.Uri) => {
const testsToBeRemoved = (fileToTestMap.get(file.toString()) || []).filter(
(t) => !testSet.has(t.id),
);
for (const test of testsToBeRemoved) {
const parentList = test.parent?.children || testController.items;
deleteTest(test, parentList);
}
};
for (const file of scopeFile) {
removeByFile(vscode.Uri.parse(file.uri));
}
};
for (const root of scope) {
recursivelyRemove(idToTestMap.get(root)!.children);
}
};

Expand Down