Skip to content

Treat test_that() tests as document symbols #856

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 21, 2025
+319 −0
Merged

Treat test_that() tests as document symbols #856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9271c48
Emit `test_that()` calls as symbols
lionel- Jun 26, 2025
1b58bb0
Expand test with multiple sections
lionel- Jun 27, 2025
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
222 changes: 222 additions & 0 deletions crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
---
source: crates/ark/src/lsp/symbols.rs
expression: "test_symbol(\"\ntest_that_not('foo', {\n 1\n})\n\n# title ----\n\ntest_that('foo', {\n # title1 ----\n 1\n # title2 ----\n foo <- function() {\n 2\n }\n})\n\n# title2 ----\ntest_that('bar', {\n 1\n})\n\")"
---
[
DocumentSymbol {
name: "title",
detail: None,
kind: String,
tags: None,
deprecated: None,
range: Range {
start: Position {
line: 5,
character: 0,
},
end: Position {
line: 15,
character: 0,
},
},
selection_range: Range {
start: Position {
line: 5,
character: 0,
},
end: Position {
line: 15,
character: 0,
},
},
children: Some(
[
DocumentSymbol {
name: "Test: foo",
detail: None,
kind: Function,
tags: None,
deprecated: None,
range: Range {
start: Position {
line: 7,
character: 0,
},
end: Position {
line: 14,
character: 2,
},
},
selection_range: Range {
start: Position {
line: 7,
character: 0,
},
end: Position {
line: 14,
character: 2,
},
},
children: Some(
[
DocumentSymbol {
name: "title1",
detail: None,
kind: String,
tags: None,
deprecated: None,
range: Range {
start: Position {
line: 8,
character: 2,
},
end: Position {
line: 9,
character: 3,
},
},
selection_range: Range {
start: Position {
line: 8,
character: 2,
},
end: Position {
line: 9,
character: 3,
},
},
children: Some(
[],
),
},
DocumentSymbol {
name: "title2",
detail: None,
kind: String,
tags: None,
deprecated: None,
range: Range {
start: Position {
line: 10,
character: 2,
},
end: Position {
line: 13,
character: 3,
},
},
selection_range: Range {
start: Position {
line: 10,
character: 2,
},
end: Position {
line: 13,
character: 3,
},
},
children: Some(
[
DocumentSymbol {
name: "foo",
detail: Some(
"function()",
),
kind: Function,
tags: None,
deprecated: None,
range: Range {
start: Position {
line: 11,
character: 2,
},
end: Position {
line: 13,
character: 3,
},
},
selection_range: Range {
start: Position {
line: 11,
character: 2,
},
end: Position {
line: 13,
character: 3,
},
},
children: Some(
[],
),
},
],
),
},
],
),
},
],
),
},
DocumentSymbol {
name: "title2",
detail: None,
kind: String,
tags: None,
deprecated: None,
range: Range {
start: Position {
line: 16,
character: 0,
},
end: Position {
line: 19,
character: 2,
},
},
selection_range: Range {
start: Position {
line: 16,
character: 0,
},
end: Position {
line: 19,
character: 2,
},
},
children: Some(
[
DocumentSymbol {
name: "Test: bar",
detail: None,
kind: Function,
tags: None,
deprecated: None,
range: Range {
start: Position {
line: 17,
character: 0,
},
end: Position {
line: 19,
character: 2,
},
},
selection_range: Range {
start: Position {
line: 17,
character: 0,
},
end: Position {
line: 19,
character: 2,
},
},
children: Some(
[],
),
},
],
),
},
]
97 changes: 97 additions & 0 deletions crates/ark/src/lsp/symbols.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@ fn collect_symbols(
collect_sections(node, contents, current_level, symbols)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely agree with our plans to remove local variables from outline, way too noisy

Image

},

NodeType::Call => {
collect_call(node, contents, symbols)?;
},

NodeType::BinaryOperator(BinaryOperatorType::LeftAssignment) |
NodeType::BinaryOperator(BinaryOperatorType::EqualsAssignment) => {
collect_assignment(node, contents, symbols)?;
Expand Down Expand Up @@ -253,6 +257,72 @@ fn collect_sections(
Ok(())
}

fn collect_call(
node: &Node,
contents: &Rope,
symbols: &mut Vec<DocumentSymbol>,
) -> anyhow::Result<()> {
let Some(callee) = node.child_by_field_name("function") else {
return Ok(());
};
if !callee.is_identifier() {
return Ok(());
}

let fun_symbol = contents.node_slice(&callee)?.to_string();

match fun_symbol.as_str() {
"test_that" => collect_call_test_that(node, contents, symbols)?,
_ => {},
}

Ok(())
}

// https://github.com/posit-dev/positron/issues/1428
fn collect_call_test_that(
node: &Node,
contents: &Rope,
symbols: &mut Vec<DocumentSymbol>,
) -> anyhow::Result<()> {
let Some(arguments) = node.child_by_field_name("arguments") else {
return Ok(());
};

// We don't do any argument matching and just consider the first argument if
// a string. First skip over `(`.
let Some(first_argument) = arguments.child(1).and_then(|n| n.child(0)) else {
return Ok(());
};
if !first_argument.is_string() {
return Ok(());
}

let Some(string) = first_argument.child_by_field_name("content") else {
return Ok(());
};

// Recurse in arguments. We could skip the first one if we wanted.
let mut children = Vec::new();
let mut cursor = arguments.walk();
for child in arguments.children_by_field_name("argument", &mut cursor) {
Comment on lines +294 to +308
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a little nicer if you pull out arguments.children_by_field_name("argument", &mut cursor) into a variable and:

  • Use the first child for the first_argument bits
  • Use the second child for collect_symbols()
  • Ignore any other children

I feel like that would make our assumption of "test_that is a 2 argument function call where the first is a string and the second is the test" more explicit and clear to see in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try and do something along these lines. In one the PRs I sent we gain some nicer iterators over arguments that should help a lot for this sort of things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just come back to this once the iterators are merged.

if let Some(value) = child.child_by_field_name("value") {
collect_symbols(&value, contents, 0, &mut children)?;
}
}

let name = contents.node_slice(&string)?.to_string();
let name = format!("Test: {name}");

let start = convert_point_to_position(contents, node.start_position());
let end = convert_point_to_position(contents, node.end_position());

let symbol = new_symbol_node(name, SymbolKind::FUNCTION, Range { start, end }, children);
symbols.push(symbol);

Ok(())
}

fn collect_assignment(
node: &Node,
contents: &Rope,
Expand Down Expand Up @@ -587,4 +657,31 @@ z <- 3",
assert_eq!(section_b.range.start.line, 4);
assert_eq!(section_b.range.end.line, 5); // End of function body
}

#[test]
fn test_symbol_call_test_that() {
insta::assert_debug_snapshot!(test_symbol(
"
test_that_not('foo', {
1
})

# title ----

test_that('foo', {
# title1 ----
1
# title2 ----
foo <- function() {
2
}
})

# title2 ----
test_that('bar', {
1
})
"
));
}
}