Skip to content

Commit

Permalink
Bring harmony to the test snapshot filtering situation (#2678)
Browse files Browse the repository at this point in the history
The snapshot filtering situation has gotten way out of hand, with each
test hand-rolling it's own filters on top of copied cruft from previous
tests.

I've attempted to address this holistically:

- `TestContext.filters()` has everything you should need 
- This was introduced a while ago, but needed a few more filters for it
to be generalized everywhere
- Using `INSTA_FILTERS` is **not recommended** unless you do not want
the context filters
    - It is okay to extend these filters for things unrelated to paths
- If you have to write a custom path filter, please highlight it in
review so we can address it in the common module
- `TestContext.site_packages()` gives cross-platform access to the
site-packages directory
    - Do not manually construct the path to site-packages from the venv
- Do not turn off tests on Windows because you manually constructed a
Unix path to site-packages
- `TestContext.workspace_root` gives access to uv's repository directory
    - Use this for installing from `scripts/packages/`
- If you need coverage for relative paths, copy the test package into
the `temp_dir` don't change the working directory of the test fixture

There is additional work that can be done here, such as:

- Auditing and removing additional uses of `INSTA_FILTERS`
- Updating manual construction of `Command` instances to use a utility
- The `venv` tests are particularly frightening in their lack of a test
context and could use some love
- Improving the developer experience i.e. apply context filters to
snapshots by default
  • Loading branch information
zanieb committed Mar 27, 2024
1 parent ffd78d0 commit 248d6f8
Show file tree
Hide file tree
Showing 15 changed files with 695 additions and 1,392 deletions.
11 changes: 9 additions & 2 deletions crates/uv-fs/src/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ use std::path::{Component, Path, PathBuf};

use once_cell::sync::Lazy;

pub static CWD: Lazy<PathBuf> = Lazy::new(|| std::env::current_dir().unwrap());
pub static CWD: Lazy<PathBuf> = Lazy::new(|| {
std::env::current_dir()
.unwrap()
.canonicalize()
.expect("The current directory must exist")
});

pub trait Simplified {
/// Simplify a [`Path`].
Expand Down Expand Up @@ -34,7 +39,9 @@ impl<T: AsRef<Path>> Simplified for T {

fn user_display(&self) -> std::path::Display {
let path = dunce::simplified(self.as_ref());
path.strip_prefix(&*CWD).unwrap_or(path).display()
path.strip_prefix(CWD.simplified())
.unwrap_or(path)
.display()
}
}

Expand Down
7 changes: 5 additions & 2 deletions crates/uv/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ fn main() {
// The workspace root directory is not available without walking up the tree
// https://github.com/rust-lang/cargo/issues/3946
let workspace_root = Path::new(&std::env::var("CARGO_MANIFEST_DIR").unwrap())
.join("..")
.join("..");
.parent()
.expect("CARGO_MANIFEST_DIR should be nested in workspace")
.parent()
.expect("CARGO_MANIFEST_DIR should be doubly nested in workspace")
.to_path_buf();

commit_info(&workspace_root);

Expand Down
53 changes: 19 additions & 34 deletions crates/uv/tests/cache_prune.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use assert_fs::prelude::*;

use common::uv_snapshot;

use crate::common::{get_bin, TestContext, INSTA_FILTERS};
use crate::common::{get_bin, TestContext};

mod common;

Expand Down Expand Up @@ -66,18 +66,13 @@ fn prune_no_op() -> Result<()> {
.assert()
.success();

let filters = [(r"Pruning cache at: .*", "Pruning cache at: [CACHE_DIR]")]
.into_iter()
.chain(INSTA_FILTERS.to_vec())
.collect::<Vec<_>>();

uv_snapshot!(filters, prune_command(&context).arg("--verbose"), @r###"
uv_snapshot!(context.filters(), prune_command(&context).arg("--verbose"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Pruning cache at: [CACHE_DIR]
Pruning cache at: [CACHE_DIR]/
No unused entries found
"###);

Expand All @@ -102,25 +97,14 @@ fn prune_stale_directory() -> Result<()> {
let simple = context.cache_dir.child("simple-v4");
simple.create_dir_all()?;

let filters = [
(r"Pruning cache at: .*", "Pruning cache at: [CACHE_DIR]"),
(
r"Removing dangling cache entry: .*[\\|/]simple-v4",
"Pruning cache at: [CACHE_DIR]/simple-v4",
),
]
.into_iter()
.chain(INSTA_FILTERS.to_vec())
.collect::<Vec<_>>();

uv_snapshot!(filters, prune_command(&context).arg("--verbose"), @r###"
uv_snapshot!(context.filters(), prune_command(&context).arg("--verbose"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Pruning cache at: [CACHE_DIR]
DEBUG Pruning cache at: [CACHE_DIR]/simple-v4
Pruning cache at: [CACHE_DIR]/
DEBUG Removing dangling cache entry: [CACHE_DIR]/simple-v4
Removed 1 directory
"###);

Expand All @@ -145,25 +129,26 @@ fn prune_stale_symlink() -> Result<()> {
let wheels = context.cache_dir.child("wheels-v0");
fs_err::remove_dir_all(wheels)?;

let filters = [
(r"Pruning cache at: .*", "Pruning cache at: [CACHE_DIR]"),
(
r"Removing dangling cache entry: .*[\\|/]archive-v0[\\|/].*",
"Pruning cache at: [CACHE_DIR]/archive-v0/anyio",
),
]
.into_iter()
.chain(INSTA_FILTERS.to_vec())
.collect::<Vec<_>>();
let filters: Vec<_> = context
.filters()
.into_iter()
.chain([
// The cache entry does not have a stable key, so we filter it out
(
r"\[CACHE_DIR\](\\|\/)(.+)(\\|\/).*",
"[CACHE_DIR]/$2/[ENTRY]",
),
])
.collect();

uv_snapshot!(filters, prune_command(&context).arg("--verbose"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Pruning cache at: [CACHE_DIR]
DEBUG Pruning cache at: [CACHE_DIR]/archive-v0/anyio
Pruning cache at: [CACHE_DIR]/
DEBUG Removing dangling cache entry: [CACHE_DIR]/archive-v0/[ENTRY]
Removed 44 files ([SIZE])
"###);

Expand Down
132 changes: 94 additions & 38 deletions crates/uv/tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub struct TestContext {
pub cache_dir: assert_fs::TempDir,
pub venv: PathBuf,
pub python_version: String,
pub workspace_root: PathBuf,

// Standard filters for this test context
filters: Vec<(String, String)>,
Expand All @@ -56,29 +57,78 @@ impl TestContext {
let cache_dir = assert_fs::TempDir::new().expect("Failed to create cache dir");
let venv = create_venv(&temp_dir, &cache_dir, python_version);

// The workspace root directory is not available without walking up the tree
// https://github.com/rust-lang/cargo/issues/3946
let workspace_root = Path::new(&std::env::var("CARGO_MANIFEST_DIR").unwrap())
.parent()
.expect("CARGO_MANIFEST_DIR should be nested in workspace")
.parent()
.expect("CARGO_MANIFEST_DIR should be doubly nested in workspace")
.to_path_buf();

let site_packages = site_packages_path(&venv, format!("python{python_version}"));

let mut filters = Vec::new();
filters.extend(
Self::path_patterns(&cache_dir)
.into_iter()
.map(|pattern| (pattern, "[CACHE_DIR]/".to_string())),
);
filters.extend(
Self::path_patterns(&temp_dir)
Self::path_patterns(&site_packages)
.into_iter()
.map(|pattern| (pattern, "[TEMP_DIR]/".to_string())),
.map(|pattern| (pattern, "[SITE_PACKAGES]/".to_string())),
);
filters.extend(
Self::path_patterns(&venv)
.into_iter()
.map(|pattern| (pattern, "[VENV]/".to_string())),
);
filters.extend(
Self::path_patterns(&temp_dir)
.into_iter()
.map(|pattern| (pattern, "[TEMP_DIR]/".to_string())),
);
filters.extend(
Self::path_patterns(&workspace_root)
.into_iter()
.map(|pattern| (pattern, "[WORKSPACE]/".to_string())),
);

// Account for [`Simplified::user_display`] which is relative to the command working directory
filters.push((
Self::path_pattern(
site_packages
.strip_prefix(&temp_dir)
.expect("The test site-packages directory is always in the tempdir"),
),
"[SITE_PACKAGES]/".to_string(),
));
filters.push((
Self::path_pattern(
venv.strip_prefix(&temp_dir)
.expect("The test virtual environment directory is always in the tempdir"),
),
"[VENV]/".to_string(),
));

// Filter non-deterministic temporary directory names
// Note we apply this _after_ all the full paths to avoid breaking their matching
filters.push((r"(\\|\/)\.tmp.*(\\|\/)".to_string(), "/[TMP]/".to_string()));

// Account for platform prefix differences `file://` (Unix) vs `file:///` (Windows)
filters.push((r"file:///".to_string(), "file://".to_string()));

// Destroy any remaining UNC prefixes (Windows only)
filters.push((r"\\\\\?\\".to_string(), String::new()));

Self {
temp_dir,
cache_dir,
venv,
python_version: python_version.to_string(),
filters,
workspace_root,
}
}

Expand Down Expand Up @@ -128,33 +178,35 @@ impl TestContext {
.stdout(version);
}

/// Generate an escaped regex pattern for the given path.
/// Generate various escaped regex patterns for the given path.
fn path_patterns(path: impl AsRef<Path>) -> Vec<String> {
vec![
format!(
// Trim the trailing separator for cross-platform directories filters
r"{}\\?/?",
regex::escape(
&path
.as_ref()
.canonicalize()
.expect("Failed to create canonical path")
// Normalize the path to match display and remove UNC prefixes on Windows
.simplified()
.display()
.to_string(),
)
// Make seprators platform agnostic because on Windows we will display
let mut patterns = Vec::new();

// We can only canonicalize paths that exist already
if path.as_ref().exists() {
patterns.push(Self::path_pattern(
path.as_ref()
.canonicalize()
.expect("Failed to create canonical path"),
));
}

// Include a non-canonicalized version
patterns.push(Self::path_pattern(path));

patterns
}

/// Generate an escaped regex pattern for the given path.
fn path_pattern(path: impl AsRef<Path>) -> String {
format!(
// Trim the trailing separator for cross-platform directories filters
r"{}\\?/?",
regex::escape(&path.as_ref().simplified_display().to_string())
// Make separators platform agnostic because on Windows we will display
// paths with Unix-style separators sometimes
.replace(r"\\", r"(\\|\/)")
),
// Include a non-canonicalized version
format!(
r"{}\\?/?",
regex::escape(&path.as_ref().simplified().display().to_string())
.replace(r"\\", r"(\\|\/)")
),
]
)
}

/// Standard snapshot filters _plus_ those for this test context.
Expand All @@ -176,16 +228,20 @@ impl TestContext {

/// Returns the site-packages folder inside the venv.
pub fn site_packages(&self) -> PathBuf {
if cfg!(unix) {
self.venv
.join("lib")
.join(format!("{}{}", self.python_kind(), self.python_version))
.join("site-packages")
} else if cfg!(windows) {
self.venv.join("Lib").join("site-packages")
} else {
unimplemented!("Only Windows and Unix are supported")
}
site_packages_path(
&self.venv,
format!("{}{}", self.python_kind(), self.python_version),
)
}
}

fn site_packages_path(venv: &Path, python: String) -> PathBuf {
if cfg!(unix) {
venv.join("lib").join(python).join("site-packages")
} else if cfg!(windows) {
venv.join("Lib").join("site-packages")
} else {
unimplemented!("Only Windows and Unix are supported")
}
}

Expand Down Expand Up @@ -253,8 +309,8 @@ pub fn bootstrapped_pythons() -> Option<Vec<PathBuf>> {

/// Create a virtual environment named `.venv` in a temporary directory with the given
/// Python version. Expected format for `python` is "python<version>".
pub fn create_venv(
temp_dir: &assert_fs::TempDir,
pub fn create_venv<Parent: assert_fs::prelude::PathChild + AsRef<std::path::Path>>(
temp_dir: &Parent,
cache_dir: &assert_fs::TempDir,
python: &str,
) -> PathBuf {
Expand Down
Loading

0 comments on commit 248d6f8

Please sign in to comment.