Skip to content

Commit 0e5674e

Browse files
tausbnCopilot
andcommitted
unified: Always run the corpus tests
The corpus tests skipped themselves when the `swift-syntax-parse` binary could not be launched. That was meant to keep the suite usable without a Swift toolchain, but it hid far more than it helped: a skip still reports `test result: ok`, and the message explaining why only appears under `cargo test -- --nocapture`. `scripts/update-corpus.sh` never set the variable that locates the parser, so the documented way to regenerate the corpus silently exercised nothing at all. Drop the guard, so a missing parser fails loudly, and make the parser easy to find so that failing is rare: - Resolve the parser one directory above the running executable as well as beside it. Test binaries live in `target/<profile>/deps/`, so the existing sibling lookup could never find `target/<profile>/swift-syntax-parse` and `cargo test` always fell through to `PATH`. - Report a missing binary with the command that builds it, rather than a bare `No such file or directory`. - Build the parser in `scripts/update-corpus.sh`, so regenerating the corpus works from a clean checkout. `cargo test` in `extractor` still needs the parser built first, since the extractor deliberately does not depend on the crate that provides it -- that is what keeps the Swift toolchain off the build path for the other languages. `AGENTS.md` now says so. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 478878c commit 0e5674e

4 files changed

Lines changed: 33 additions & 51 deletions

File tree

unified/AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ This is a CodeQL extractor based on tree-sitter.
1717

1818
- The mapping from the parse tree to the target AST is found in `extractor/src/languages/swift/swift.rs`
1919

20-
- To run tests for the parser and mapping, run `cargo test` in the `extractor` directory.
20+
- To run tests for the parser and mapping, run `cargo test` in the `extractor` directory. The corpus tests shell out to the `swift-syntax-parse` binary, which lives in a separate crate that `cargo test` does not build, so build it first with `cargo build -p swift-syntax-rs --bin swift-syntax-parse` (this needs a Swift toolchain; `scripts/update-corpus.sh` does it for you).
2121

2222
- Extractor test cases are located at `extractor/tests/corpus/swift/*/*.swift`.
2323

unified/extractor/src/languages/swift/parse.rs

Lines changed: 25 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -43,46 +43,43 @@ pub fn parse(source: &[u8]) -> Result<ParsedTree, String> {
4343
/// extractor pack lays it out (`tools/<platform>/{extractor,
4444
/// swift-syntax-parse}`), so a packaged extractor is self-contained with no
4545
/// environment setup;
46-
/// 3. a bare `swift-syntax-parse`, looked up on `PATH`.
46+
/// 3. a copy one directory further up, which is where `cargo` leaves it when
47+
/// the running executable is a test binary: those live in
48+
/// `target/<profile>/deps/`, one level below `target/<profile>/`;
49+
/// 4. a bare `swift-syntax-parse`, looked up on `PATH`.
4750
fn parse_bin() -> String {
4851
if let Ok(bin) = std::env::var(PARSE_BIN_ENV) {
4952
if !bin.is_empty() {
5053
return bin;
5154
}
5255
}
5356
if let Ok(exe) = std::env::current_exe() {
54-
if let Some(sibling) = exe.parent().map(|dir| dir.join(PARSE_BIN_NAME)) {
55-
if sibling.is_file() {
56-
return sibling.to_string_lossy().into_owned();
57+
let exe_dir = exe.parent();
58+
let candidates = [exe_dir, exe_dir.and_then(|dir| dir.parent())];
59+
for dir in candidates.into_iter().flatten() {
60+
let candidate = dir.join(PARSE_BIN_NAME);
61+
if candidate.is_file() {
62+
return candidate.to_string_lossy().into_owned();
5763
}
5864
}
5965
}
6066
PARSE_BIN_NAME.to_string()
6167
}
6268

63-
/// Whether the `swift-syntax-parse` executable can be launched at all.
64-
///
65-
/// This reports availability of the *executable*, deliberately not whether
66-
/// parsing succeeds: a binary that launches but then crashes or emits invalid
67-
/// JSON is still "available", so callers run and surface the failure rather
68-
/// than silently skipping. Only a genuinely missing/unlaunchable binary (e.g.
69-
/// no Swift toolchain is installed) reports `false`.
70-
pub fn binary_available() -> bool {
71-
match Command::new(parse_bin())
72-
.stdin(Stdio::null())
73-
.stdout(Stdio::null())
74-
.stderr(Stdio::null())
75-
.spawn()
76-
{
77-
Ok(mut child) => {
78-
let _ = child.wait();
79-
true
80-
}
81-
Err(e) if e.kind() == std::io::ErrorKind::NotFound => false,
82-
// Any other spawn failure (e.g. a permissions problem) is a genuine
83-
// issue worth surfacing, so treat the parser as available and let the
84-
// caller fail rather than masking it as "unavailable".
85-
Err(_) => true,
69+
/// Explain a failure to launch the parser. A missing binary is by far the most
70+
/// common way this fails — the Swift half of the build is separate, so it is
71+
/// easy to have never built it — so that case carries the remedy rather than a
72+
/// bare OS error.
73+
fn spawn_error(bin: &str, error: std::io::Error) -> String {
74+
if error.kind() == std::io::ErrorKind::NotFound {
75+
format!(
76+
"could not find the Swift parser `{bin}`. Build it with \
77+
`cargo build -p swift-syntax-rs --bin swift-syntax-parse` (this needs a Swift \
78+
toolchain — see `unified/swift-syntax-rs/.swift-version` for the pinned version), \
79+
or point `{PARSE_BIN_ENV}` at an existing copy."
80+
)
81+
} else {
82+
format!("failed to spawn Swift parser `{bin}`: {error}")
8683
}
8784
}
8885

@@ -95,7 +92,7 @@ fn run_parser(source: &str) -> Result<String, String> {
9592
.stdout(Stdio::piped())
9693
.stderr(Stdio::piped())
9794
.spawn()
98-
.map_err(|e| format!("failed to spawn Swift parser `{bin}`: {e}"))?;
95+
.map_err(|e| spawn_error(&bin, e))?;
9996

10097
// The parser reads all of stdin before writing any stdout, so writing the
10198
// whole source and then closing stdin (by dropping it) cannot deadlock.

unified/extractor/tests/corpus_tests.rs

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -20,20 +20,6 @@ fn update_mode_enabled() -> bool {
2020
.unwrap_or(false)
2121
}
2222

23-
/// Whether the external swift-syntax parser is available. When the parser
24-
/// binary genuinely cannot be found/launched (e.g. no Swift toolchain, and
25-
/// neither `CODEQL_EXTRACTOR_UNIFIED_SWIFT_SYNTAX_PARSE` nor a `swift-syntax-parse`
26-
/// on `PATH`), the corpus test is skipped rather than failed — it cannot run
27-
/// without the Swift-backed parser.
28-
///
29-
/// Crucially this checks only that the executable *launches*: a parser that is
30-
/// present but crashes, emits invalid JSON, or otherwise regresses is
31-
/// considered available, so the suite runs and fails (rather than silently
32-
/// skipping the very failures CI needs to catch).
33-
fn parser_available() -> bool {
34-
languages::swift_parse::binary_available()
35-
}
36-
3723
/// Parse a corpus `.output` file. The file holds a single test case made of
3824
/// three sections separated by `---` delimiter lines:
3925
///
@@ -112,14 +98,6 @@ fn collect_corpus_stems(dir: &Path, out: &mut Vec<std::path::PathBuf>) {
11298

11399
#[test]
114100
fn test_corpus() {
115-
if !parser_available() {
116-
eprintln!(
117-
"skipping test_corpus: the swift-syntax parser is unavailable \
118-
(set CODEQL_EXTRACTOR_UNIFIED_SWIFT_SYNTAX_PARSE or put \
119-
`swift-syntax-parse` on PATH)"
120-
);
121-
return;
122-
}
123101
let update_mode = update_mode_enabled();
124102
let all_languages = languages::all_language_specs();
125103
let corpus_dir = Path::new("tests/corpus");

unified/scripts/update-corpus.sh

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,12 @@ IFS=$'\n\t'
44

55
cd "$(dirname "$0")/.."
66

7+
# The corpus is produced by the external swift-syntax parser. That parser is a
8+
# separate crate which `cargo test` does not build (the extractor deliberately
9+
# does not depend on it, so working on other languages needs no Swift
10+
# toolchain), so build it up front — otherwise the tests below fail on a
11+
# missing binary.
12+
cargo build -p swift-syntax-rs --bin swift-syntax-parse
13+
714
cd extractor
815
UNIFIED_UPDATE_CORPUS=1 cargo test

0 commit comments

Comments
 (0)