Skip to content

Commit ad211d0

Browse files
authored
Make cargo pgrx regress <testname> use the default pg version (pgcentralfoundation#2278)
`cargo pgrx regress` was stricter than the other pgrx subcommands when it parsed positional arguments. It treated anything that looked like `pgNN` as a Postgres version, and in the two-argument case it required the first argument to be a version. That made the command behave poorly when the user wanted to run a single regression test and rely on the extension's default `pgXX` feature from `Cargo.toml`. Refactor the regress positional parsing into a small helper and tighten the version check so only supported `pgXX` labels count as explicit Postgres versions. With that change: - `cargo pgrx regress <testname>` uses the manifest default pg version - `cargo pgrx regress pg16 <testname>` still selects an explicit version - `cargo pgrx regress <testname> pg16` is still rejected - unsupported labels like `pg99` fall back to a test filter instead of being misclassified as a version Add unit tests to cover the single-test case, the explicit `pgXX <test>` case, the rejected reversed-order case, and the unsupported-`pgXX` case.
1 parent af589dd commit ad211d0

1 file changed

Lines changed: 77 additions & 29 deletions

File tree

cargo-pgrx/src/command/regress.rs

Lines changed: 77 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use crate::command::run::Run;
1313
use crate::command::start::collect_postgresql_conf_settings;
1414
use crate::manifest::get_package_manifest;
1515
use owo_colors::OwoColorize;
16-
use pgrx_pg_config::{PgConfig, createdb, dropdb};
16+
use pgrx_pg_config::{PgConfig, createdb, dropdb, is_supported_major_version};
1717
use std::collections::HashSet;
1818
use std::env::temp_dir;
1919
use std::fs::{DirEntry, File};
@@ -486,38 +486,39 @@ impl Regress {
486486
/// - `cargo pgrx regress pg16` → pg_version = Some("pg16")
487487
/// - `cargo pgrx regress pg16 my_test` → pg_version = Some("pg16"), test_filter = Some("my_test")
488488
fn resolve_args(&mut self) {
489-
fn looks_like_pg_version(s: &str) -> bool {
490-
s.starts_with("pg") && s[2..].chars().all(|c| c.is_ascii_digit()) && s.len() > 2
489+
match Self::parse_args(&self.args) {
490+
Ok((pg_version, test_filter)) => {
491+
self.pg_version = pg_version;
492+
self.test_filter = test_filter;
493+
}
494+
Err(message) => {
495+
eprintln!("{} {message}", " ERROR".bold().red());
496+
std::process::exit(1);
497+
}
491498
}
499+
}
492500

493-
match self.args.len() {
494-
0 => {}
495-
1 => {
496-
if looks_like_pg_version(&self.args[0]) {
497-
self.pg_version = Some(self.args[0].clone());
498-
} else {
499-
self.test_filter = Some(self.args[0].clone());
500-
}
501-
}
502-
2 => {
503-
if looks_like_pg_version(&self.args[0]) {
504-
self.pg_version = Some(self.args[0].clone());
505-
self.test_filter = Some(self.args[1].clone());
506-
} else {
507-
eprintln!(
508-
"{} first positional argument must be a PostgreSQL version (e.g., pg16), got `{}`",
509-
" ERROR".bold().red(),
510-
self.args[0],
511-
);
512-
std::process::exit(1);
513-
}
501+
fn parse_args(args: &[String]) -> Result<(Option<String>, Option<String>), String> {
502+
fn is_supported_pg_version_label(label: &str) -> bool {
503+
label
504+
.strip_prefix("pg")
505+
.and_then(|major| major.parse::<u16>().ok())
506+
.is_some_and(is_supported_major_version)
507+
}
508+
509+
match args {
510+
[] => Ok((None, None)),
511+
[only] if is_supported_pg_version_label(only) => Ok((Some(only.clone()), None)),
512+
[only] => Ok((None, Some(only.clone()))),
513+
[first, second] if is_supported_pg_version_label(first) => {
514+
Ok((Some(first.clone()), Some(second.clone())))
514515
}
516+
[first, _second] => Err(format!(
517+
"first positional argument must be a PostgreSQL version (e.g., pg16), got `{first}`"
518+
)),
515519
_ => {
516-
eprintln!(
517-
"{} too many positional arguments. Usage: cargo pgrx regress [pgXX] [testname]",
518-
" ERROR".bold().red(),
519-
);
520-
std::process::exit(1);
520+
Err("too many positional arguments. Usage: cargo pgrx regress [pgXX] [testname]"
521+
.into())
521522
}
522523
}
523524
}
@@ -1002,3 +1003,50 @@ fn is_git_repo(git: &Path) -> bool {
10021003
.map(|status| status.success())
10031004
.unwrap_or_default()
10041005
}
1006+
1007+
#[cfg(test)]
1008+
mod tests {
1009+
use super::Regress;
1010+
1011+
fn strings(args: &[&str]) -> Vec<String> {
1012+
args.iter().map(|arg| (*arg).to_string()).collect()
1013+
}
1014+
1015+
#[test]
1016+
fn parse_args_treats_single_non_version_as_test_filter() {
1017+
let (pg_version, test_filter) = Regress::parse_args(&strings(&["cursor_coverage"]))
1018+
.expect("single test name should parse");
1019+
1020+
assert_eq!(pg_version, None);
1021+
assert_eq!(test_filter.as_deref(), Some("cursor_coverage"));
1022+
}
1023+
1024+
#[test]
1025+
fn parse_args_accepts_pg_version_then_test_filter() {
1026+
let (pg_version, test_filter) = Regress::parse_args(&strings(&["pg16", "cursor_coverage"]))
1027+
.expect("pg version plus test filter should parse");
1028+
1029+
assert_eq!(pg_version.as_deref(), Some("pg16"));
1030+
assert_eq!(test_filter.as_deref(), Some("cursor_coverage"));
1031+
}
1032+
1033+
#[test]
1034+
fn parse_args_rejects_test_filter_then_pg_version() {
1035+
let err = Regress::parse_args(&strings(&["cursor_coverage", "pg16"]))
1036+
.expect_err("reversed positional order should still fail");
1037+
1038+
assert_eq!(
1039+
err,
1040+
"first positional argument must be a PostgreSQL version (e.g., pg16), got `cursor_coverage`"
1041+
);
1042+
}
1043+
1044+
#[test]
1045+
fn parse_args_does_not_treat_unsupported_pg_label_as_version() {
1046+
let (pg_version, test_filter) = Regress::parse_args(&strings(&["pg99"]))
1047+
.expect("unsupported pg label should fall back to test filter");
1048+
1049+
assert_eq!(pg_version, None);
1050+
assert_eq!(test_filter.as_deref(), Some("pg99"));
1051+
}
1052+
}

0 commit comments

Comments
 (0)