From 8e635c9ebfa3c8f61e6aed389ff26902853a53b5 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 13 Mar 2026 11:08:06 -0700 Subject: [PATCH 1/2] [spr] initial version Created using spr 1.3.6-beta.1 --- crates/dropshot-api-manager/src/cmd/check.rs | 24 +- crates/dropshot-api-manager/src/resolved.rs | 216 +++++++++- crates/dropshot-api-manager/src/test_util.rs | 74 +++- .../tests/integration/generated_from_dir.rs | 90 ++++- .../tests/integration/git_stub.rs | 316 ++++++++++++++- .../tests/integration/lockstep.rs | 16 +- .../tests/integration/versioned.rs | 380 ++++++++++++++++-- 7 files changed, 1043 insertions(+), 73 deletions(-) diff --git a/crates/dropshot-api-manager/src/cmd/check.rs b/crates/dropshot-api-manager/src/cmd/check.rs index e0ff42d..fe1ab49 100644 --- a/crates/dropshot-api-manager/src/cmd/check.rs +++ b/crates/dropshot-api-manager/src/cmd/check.rs @@ -7,7 +7,7 @@ use crate::{ CheckResult, OutputOpts, display_load_problems, display_resolution, headers::*, }, - resolved::Resolved, + resolved::{ProblemSummary, Resolved}, }; pub(crate) fn check_impl( @@ -17,6 +17,23 @@ pub(crate) fn check_impl( generated_source: &GeneratedSource, output: &OutputOpts, ) -> anyhow::Result { + let (result, _summaries) = check_impl_with_summaries( + apis, + env, + blessed_source, + generated_source, + output, + )?; + Ok(result) +} + +pub(crate) fn check_impl_with_summaries( + apis: &ManagedApis, + env: &ResolvedEnv, + blessed_source: &BlessedSource, + generated_source: &GeneratedSource, + output: &OutputOpts, +) -> anyhow::Result<(CheckResult, Vec)> { let styles = output.styles(supports_color::Stream::Stderr); eprintln!("{:>HEADER_WIDTH$}", SEPARATOR); @@ -38,6 +55,9 @@ pub(crate) fn check_impl( eprintln!("{:>HEADER_WIDTH$}", SEPARATOR); let result = display_resolution(env, apis, &resolved, &styles)?; + // Extract owned summaries before dropping the borrowed resolved state. + let summaries = resolved.problem_summaries(); + // Release borrows held by `resolved`, then drop the source // collections in parallel. Each contains many parsed OpenAPI // documents whose sequential drops are costly. @@ -48,5 +68,5 @@ pub(crate) fn check_impl( s.spawn(|| drop(local_files)); }); - Ok(result) + Ok((result, summaries)) } diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index 5947b71..60b4137 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -132,6 +132,77 @@ impl Display for ResolutionKind { } } +/// Identifies the kind of a `Problem` without carrying borrowed data. +/// +/// Each variant corresponds 1:1 to a `Problem` variant. The exhaustive +/// match in `Problem::kind` ensures that adding a new `Problem` variant +/// without updating this enum causes a compile error. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] +#[expect(missing_docs)] +pub enum ProblemKind { + LocalSpecFileOrphaned, + UnparseableLocalFile, + BlessedVersionMissingLocal, + BlessedVersionExtraLocalSpec, + BlessedVersionCompareError, + BlessedVersionBroken, + BlessedLatestVersionBytewiseMismatch, + LockstepMissingLocal, + LockstepStale, + LocalVersionMissingLocal, + LocalVersionExtra, + LocalVersionStale, + GeneratedSourceMissing, + GeneratedValidationError, + ExtraFileStale, + LatestLinkMissing, + LatestLinkStale, + BlessedVersionShouldBeGitStub, + GitStubShouldBeJson, + BlessedVersionCorruptedLocal, + DuplicateLocalFile, + GitStubCommitStale, + GitStubFirstCommitUnknown, +} + +/// Owned summary of a `Problem` for test assertions. +/// +/// Contains just enough information to identify a problem: which API it +/// belongs to, which version (if any), and its [`ProblemKind`]. Because all +/// fields are owned and implement `PartialEq`, summaries can be compared +/// with `assert_eq!`. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ProblemSummary { + /// The API this problem is associated with. + pub api_ident: ApiIdent, + /// The version this problem is associated with, or `None` for + /// non-version-specific problems (e.g. orphaned files, symlinks). + pub version: Option, + /// The kind of problem. + pub kind: ProblemKind, +} + +impl ProblemSummary { + /// Creates a new problem summary for a version-specific problem. + pub fn new(api_ident: &str, version: &str, kind: ProblemKind) -> Self { + ProblemSummary { + api_ident: ApiIdent::from(api_ident), + version: Some(version.parse().expect("valid semver")), + kind, + } + } + + /// Creates a new problem summary for a non-version-specific problem + /// (e.g. symlink issues, orphaned files). + pub fn for_api(api_ident: &str, kind: ProblemKind) -> Self { + ProblemSummary { + api_ident: ApiIdent::from(api_ident), + version: None, + kind, + } + } +} + /// Describes a problem resolving the blessed spec(s), generated spec(s), and /// local spec files for a particular API. #[derive(Debug, Error)] @@ -363,6 +434,72 @@ pub enum Problem<'a> { } impl<'a> Problem<'a> { + /// Returns the discriminant of this problem as a [`ProblemKind`]. + /// + /// The match is exhaustive (no wildcard), so adding a new `Problem` + /// variant without updating this method causes a compile error. + pub fn kind(&self) -> ProblemKind { + match self { + Problem::LocalSpecFileOrphaned { .. } => { + ProblemKind::LocalSpecFileOrphaned + } + Problem::UnparseableLocalFile { .. } => { + ProblemKind::UnparseableLocalFile + } + Problem::BlessedVersionMissingLocal { .. } => { + ProblemKind::BlessedVersionMissingLocal + } + Problem::BlessedVersionExtraLocalSpec { .. } => { + ProblemKind::BlessedVersionExtraLocalSpec + } + Problem::BlessedVersionCompareError { .. } => { + ProblemKind::BlessedVersionCompareError + } + Problem::BlessedVersionBroken { .. } => { + ProblemKind::BlessedVersionBroken + } + Problem::BlessedLatestVersionBytewiseMismatch { .. } => { + ProblemKind::BlessedLatestVersionBytewiseMismatch + } + Problem::LockstepMissingLocal { .. } => { + ProblemKind::LockstepMissingLocal + } + Problem::LockstepStale { .. } => ProblemKind::LockstepStale, + Problem::LocalVersionMissingLocal { .. } => { + ProblemKind::LocalVersionMissingLocal + } + Problem::LocalVersionExtra { .. } => ProblemKind::LocalVersionExtra, + Problem::LocalVersionStale { .. } => ProblemKind::LocalVersionStale, + Problem::GeneratedSourceMissing { .. } => { + ProblemKind::GeneratedSourceMissing + } + Problem::GeneratedValidationError { .. } => { + ProblemKind::GeneratedValidationError + } + Problem::ExtraFileStale { .. } => ProblemKind::ExtraFileStale, + Problem::LatestLinkMissing { .. } => ProblemKind::LatestLinkMissing, + Problem::LatestLinkStale { .. } => ProblemKind::LatestLinkStale, + Problem::BlessedVersionShouldBeGitStub { .. } => { + ProblemKind::BlessedVersionShouldBeGitStub + } + Problem::GitStubShouldBeJson { .. } => { + ProblemKind::GitStubShouldBeJson + } + Problem::BlessedVersionCorruptedLocal { .. } => { + ProblemKind::BlessedVersionCorruptedLocal + } + Problem::DuplicateLocalFile { .. } => { + ProblemKind::DuplicateLocalFile + } + Problem::GitStubCommitStale { .. } => { + ProblemKind::GitStubCommitStale + } + Problem::GitStubFirstCommitUnknown { .. } => { + ProblemKind::GitStubFirstCommitUnknown + } + } + } + pub fn is_fixable(&self) -> bool { self.fix().is_some() } @@ -914,7 +1051,7 @@ fn symlink_file(target: &str, path: &Utf8Path) -> std::io::Result<()> { /// local spec files for a given API pub struct Resolved<'a> { notes: Vec, - non_version_problems: Vec>, + non_version_problems: Vec<(ApiIdent, Option, Problem<'a>)>, api_results: BTreeMap>, nexpected_documents: usize, } @@ -961,12 +1098,23 @@ impl<'a> Resolved<'a> { // Get the other easy case out of the way: if there are any local spec // files for APIs or API versions that aren't supported any more, that's // a (fixable) problem. - let mut non_version_problems: Vec> = - resolve_orphaned_local_specs(&supported_versions_by_api, local) - .map(|spec_file_name| Problem::LocalSpecFileOrphaned { - spec_file_name: spec_file_name.clone(), - }) - .collect(); + let mut non_version_problems: Vec<( + ApiIdent, + Option, + Problem<'_>, + )> = resolve_orphaned_local_specs(&supported_versions_by_api, local) + .map(|spec_file_name| { + let ident = spec_file_name.ident().clone(); + let version = Some(spec_file_name.version().clone()); + ( + ident, + version, + Problem::LocalSpecFileOrphaned { + spec_file_name: spec_file_name.clone(), + }, + ) + }) + .collect(); // Resolve each of the supported API versions first, so we know what // paths will be written. (Do this in parallel across each API version.) @@ -1039,13 +1187,17 @@ impl<'a> Resolved<'a> { } } - for (_ident, api_files) in local.iter() { + for (ident, api_files) in local.iter() { for unparseable in api_files.unparseable_files() { // Only report if no fix will overwrite this path. if !paths_written.contains(&unparseable.path) { - non_version_problems.push(Problem::UnparseableLocalFile { - unparseable_file: unparseable.clone(), - }); + non_version_problems.push(( + ident.clone(), + None, + Problem::UnparseableLocalFile { + unparseable_file: unparseable.clone(), + }, + )); } } } @@ -1067,7 +1219,7 @@ impl<'a> Resolved<'a> { } pub fn general_problems(&self) -> impl Iterator> + '_ { - self.non_version_problems.iter() + self.non_version_problems.iter().map(|(_, _, problem)| problem) } pub fn resolution_for_api_version( @@ -1086,6 +1238,46 @@ impl<'a> Resolved<'a> { self.general_problems().any(|p| !p.is_fixable()) || self.api_results.values().any(|a| a.has_unfixable_problems()) } + + /// Returns an owned, ordered list of all problems as summaries. + /// + /// Order: general (non-version-specific) problems first, then per-API + /// (sorted by ident), per-version (sorted by semver), then symlink + /// problems. + pub fn problem_summaries(&self) -> Vec { + let mut summaries = Vec::new(); + + // General problems. + for (ident, version, problem) in &self.non_version_problems { + summaries.push(ProblemSummary { + api_ident: ident.clone(), + version: version.clone(), + kind: problem.kind(), + }); + } + + // Per-API problems. + for (ident, api_resolved) in &self.api_results { + for (version, resolution) in &api_resolved.by_version { + for problem in resolution.problems() { + summaries.push(ProblemSummary { + api_ident: ident.clone(), + version: Some(version.clone()), + kind: problem.kind(), + }); + } + } + if let Some(symlink) = &api_resolved.symlink { + summaries.push(ProblemSummary { + api_ident: ident.clone(), + version: None, + kind: symlink.kind(), + }); + } + } + + summaries + } } struct ApiResolved<'a> { diff --git a/crates/dropshot-api-manager/src/test_util.rs b/crates/dropshot-api-manager/src/test_util.rs index cd3eda8..48a315a 100644 --- a/crates/dropshot-api-manager/src/test_util.rs +++ b/crates/dropshot-api-manager/src/test_util.rs @@ -3,14 +3,17 @@ //! Test utilities for the Dropshot API manager. pub use crate::output::CheckResult; +#[doc(hidden)] +pub use crate::resolved::{ProblemKind, ProblemSummary}; use crate::{ apis::ManagedApis, cmd::{ - check::check_impl, + check::check_impl_with_summaries, dispatch::{BlessedSourceArgs, GeneratedSourceArgs}, }, environment::{Environment, GeneratedSource}, output::OutputOpts, + resolved, }; use camino::Utf8PathBuf; @@ -21,7 +24,8 @@ pub fn check_apis_up_to_date( env: &Environment, apis: &ManagedApis, ) -> Result { - check_apis_impl(env, apis, None) + let (result, _summaries) = check_apis_with_summaries(env, apis)?; + Ok(result) } /// Check that a set of APIs is up-to-date, loading generated documents from @@ -31,28 +35,78 @@ pub fn check_apis_with_generated_from_dir( apis: &ManagedApis, generated_from_dir: Utf8PathBuf, ) -> Result { - check_apis_impl(env, apis, Some(generated_from_dir)) + let (result, _summaries) = + check_apis_with_generated_from_dir_and_summaries( + env, + apis, + generated_from_dir, + )?; + Ok(result) } -fn check_apis_impl( +/// Like [`check_apis_up_to_date`], but also returns the list of problem +/// summaries for detailed assertions in tests. +#[doc(hidden)] +pub fn check_apis_with_summaries( env: &Environment, apis: &ManagedApis, - generated_from_dir: Option, -) -> Result { +) -> Result<(CheckResult, Vec), anyhow::Error> { + let env = resolve_env(env)?; + let (blessed_source, generated_source, output) = + default_sources(&env, None)?; + check_impl_with_summaries( + apis, + &env, + &blessed_source, + &generated_source, + &output, + ) +} + +/// Like [`check_apis_with_generated_from_dir`], but also returns the list +/// of problem summaries for detailed assertions in tests. +#[doc(hidden)] +pub fn check_apis_with_generated_from_dir_and_summaries( + env: &Environment, + apis: &ManagedApis, + generated_from_dir: Utf8PathBuf, +) -> Result<(CheckResult, Vec), anyhow::Error> { + let env = resolve_env(env)?; + let (blessed_source, generated_source, output) = + default_sources(&env, Some(generated_from_dir))?; + check_impl_with_summaries( + apis, + &env, + &blessed_source, + &generated_source, + &output, + ) +} + +fn resolve_env( + env: &Environment, +) -> Result { // env.resolve(None) assumes that env.default_openapi_dir is where the // OpenAPI documents live and doesn't need a further override. (If a custom // directory is desired, it can always be passed in via `env`.) - let env = env.resolve(None)?; + env.resolve(None) +} +fn default_sources( + env: &crate::environment::ResolvedEnv, + generated_from_dir: Option, +) -> Result< + (crate::environment::BlessedSource, GeneratedSource, OutputOpts), + anyhow::Error, +> { let blessed_source = BlessedSourceArgs { blessed_from_vcs: None, blessed_from_vcs_path: None, blessed_from_dir: None, } - .to_blessed_source(&env)?; + .to_blessed_source(env)?; let generated_source = GeneratedSource::from(GeneratedSourceArgs { generated_from_dir }); let output = OutputOpts { color: clap::ColorChoice::Auto }; - - check_impl(apis, &env, &blessed_source, &generated_source, &output) + Ok((blessed_source, generated_source, output)) } diff --git a/crates/integration-tests/tests/integration/generated_from_dir.rs b/crates/integration-tests/tests/integration/generated_from_dir.rs index fff3450..20a1877 100644 --- a/crates/integration-tests/tests/integration/generated_from_dir.rs +++ b/crates/integration-tests/tests/integration/generated_from_dir.rs @@ -5,7 +5,9 @@ use anyhow::Result; use atomicwrites::{AtomicFile, OverwriteBehavior}; use dropshot_api_manager::test_util::{ - CheckResult, check_apis_up_to_date, check_apis_with_generated_from_dir, + CheckResult, ProblemKind, ProblemSummary, check_apis_up_to_date, + check_apis_with_generated_from_dir, + check_apis_with_generated_from_dir_and_summaries, }; use integration_tests::*; use std::io::Write; @@ -32,12 +34,32 @@ fn test_generated_from_empty_dir_does_not_panic() -> Result<()> { // The missing API should be reported as an unfixable problem, not a // panic. - let result = check_apis_with_generated_from_dir( + let (result, summaries) = check_apis_with_generated_from_dir_and_summaries( env.environment(), &apis, empty_dir, )?; assert_eq!(result, CheckResult::Failures); + assert_eq!( + summaries, + vec![ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ], + ); Ok(()) } @@ -58,12 +80,52 @@ fn test_generated_from_partial_dir_does_not_panic() -> Result<()> { // The versioned APIs have no generated source, so they should be // reported as failures. - let result = check_apis_with_generated_from_dir( + let (result, summaries) = check_apis_with_generated_from_dir_and_summaries( env.environment(), &apis, partial_dir, )?; assert_eq!(result, CheckResult::Failures); + assert_eq!( + summaries, + vec![ + ProblemSummary::new( + "counter", + "1.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-user", + "1.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-user", + "2.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-user", + "3.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ], + ); Ok(()) } @@ -105,12 +167,32 @@ fn test_generated_from_dir_partial_versions() -> Result<()> { // Should not panic. Some versions are missing, so the result should // be Failures. - let result = check_apis_with_generated_from_dir( + let (result, summaries) = check_apis_with_generated_from_dir_and_summaries( env.environment(), &apis_v4, gen_dir, )?; assert_eq!(result, CheckResult::Failures); + assert_eq!( + summaries, + vec![ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::GeneratedSourceMissing, + ), + ], + ); Ok(()) } diff --git a/crates/integration-tests/tests/integration/git_stub.rs b/crates/integration-tests/tests/integration/git_stub.rs index 2df2356..1f8a135 100644 --- a/crates/integration-tests/tests/integration/git_stub.rs +++ b/crates/integration-tests/tests/integration/git_stub.rs @@ -11,7 +11,10 @@ use anyhow::{Context, Result}; use camino::Utf8PathBuf; use dropshot_api_manager::{ ManagedApis, - test_util::{CheckResult, check_apis_up_to_date}, + test_util::{ + CheckResult, ProblemKind, ProblemSummary, check_apis_up_to_date, + check_apis_with_summaries, + }, }; use integration_tests::{ ExpectedConflictKind, ExpectedConflicts, all_conflict_paths, @@ -268,12 +271,28 @@ fn test_mixed_first_commits_selective_conversion() -> Result<()> { // v3 is latest (from second_commit) while v1, v2 are from first_commit. let v1_v2_v3_git_stub = versioned_health_git_stub_apis()?; - let result = check_apis_up_to_date(env.environment(), &v1_v2_v3_git_stub)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v1_v2_v3_git_stub)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should suggest converting v1, v2 (different first commit)" ); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::BlessedVersionShouldBeGitStub, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::BlessedVersionShouldBeGitStub, + ), + ], + ); env.generate_documents(&v1_v2_v3_git_stub)?; @@ -444,14 +463,36 @@ fn test_convert_to_json_when_disabled() -> Result<()> { ); let extended_without_git_stub = versioned_health_with_v4_apis()?; - let result = - check_apis_up_to_date(env.environment(), &extended_without_git_stub)?; + let (result, summaries) = check_apis_with_summaries( + env.environment(), + &extended_without_git_stub, + )?; assert_eq!( result, CheckResult::NeedsUpdate, "check should report needs update when Git stubs exist but Git stub \ storage is disabled" ); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::GitStubShouldBeJson, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::GitStubShouldBeJson, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::GitStubShouldBeJson, + ), + ], + ); env.generate_documents(&extended_without_git_stub)?; @@ -547,12 +588,21 @@ fn test_duplicates() -> Result<()> { "duplicate JSON should exist" ); - let result = check_apis_up_to_date(env.environment(), &extended)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &extended)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should report needs update when duplicate files exist" ); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::DuplicateLocalFile, + )], + ); env.generate_documents(&extended)?; @@ -858,11 +908,41 @@ fn test_git_error_reports_problem() -> Result<()> { } let v4_apis = versioned_health_with_v4_git_stub_apis()?; - let result = check_apis_up_to_date(env.environment(), &v4_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_apis)?; // Should report a failure due to the unfixable GitStubFirstCommitUnknown // problem. assert_eq!(result, CheckResult::Failures); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::GitStubFirstCommitUnknown, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::GitStubFirstCommitUnknown, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::GitStubFirstCommitUnknown, + ), + ProblemSummary::new( + "versioned-health", + "4.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkStale, + ), + ], + ); Ok(()) } @@ -1701,12 +1781,21 @@ def456:documents/versioned-health/new.json\n\ >>>>>>> branch\n"; env.create_file(&v1_git_stub_path, corrupted_content)?; - let result = check_apis_up_to_date(env.environment(), &v1_v2_v3_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v1_v2_v3_apis)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should report needs update for unparseable Git stub" ); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::BlessedVersionCorruptedLocal, + )], + ); env.generate_documents(&v1_v2_v3_apis)?; @@ -1770,12 +1859,21 @@ fn test_non_canonical_git_stub_regenerated() -> Result<()> { let non_canonical_content = format!("{}:{}", commit, non_canonical_path); env.create_file(&v1_git_stub_path, &non_canonical_content)?; - let result = check_apis_up_to_date(env.environment(), &v1_v2_v3_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v1_v2_v3_apis)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should report needs update for non-canonical Git stub" ); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::BlessedVersionCorruptedLocal, + )], + ); env.generate_documents(&v1_v2_v3_apis)?; @@ -1824,12 +1922,21 @@ fn test_empty_git_stub_regenerated() -> Result<()> { .expect("v1 Git stub should exist"); env.create_file(&v1_git_stub_path, "")?; - let result = check_apis_up_to_date(env.environment(), &v1_v2_v3_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v1_v2_v3_apis)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should report needs update for empty Git stub" ); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::BlessedVersionCorruptedLocal, + )], + ); env.generate_documents(&v1_v2_v3_apis)?; @@ -1868,12 +1975,21 @@ fn test_invalid_commit_hash_git_stub_regenerated() -> Result<()> { "not-a-valid-hash:documents/versioned-health/file.json\n"; env.create_file(&v1_git_stub_path, invalid_content)?; - let result = check_apis_up_to_date(env.environment(), &v1_v2_v3_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v1_v2_v3_apis)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should report needs update for invalid commit hash" ); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::BlessedVersionCorruptedLocal, + )], + ); env.generate_documents(&v1_v2_v3_apis)?; @@ -1926,13 +2042,22 @@ fn test_unresolvable_git_stub_regenerated() -> Result<()> { env.create_file(&v1_git_stub_path, &unresolvable_content)?; // Check should report NeedsUpdate (not Failures). - let result = check_apis_up_to_date(env.environment(), &v1_v2_v3_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v1_v2_v3_apis)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should report needs update for unresolvable Git stub, \ not a hard failure" ); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::BlessedVersionCorruptedLocal, + )], + ); // Generate should fix the problem by deleting and recreating the gitstub. env.generate_documents(&v1_v2_v3_apis)?; @@ -2636,12 +2761,33 @@ fn stale_git_stub_commit_impl(env: &mut TestEnvironment) -> Result<()> { } // Step 6: check should detect the stale commits. - let result = check_apis_up_to_date(env.environment(), &v4)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should detect stale Git stub commits" ); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::GitStubCommitStale, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::GitStubCommitStale, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::GitStubCommitStale, + ), + ], + ); // Step 7: generate should fix the stale Git stubs. env.generate_documents(&v4)?; @@ -2730,12 +2876,28 @@ fn test_stale_git_stub_commit_with_duplicate() -> Result<()> { ); // Check should detect both issues. - let result = check_apis_up_to_date(env.environment(), &v4)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4)?; assert_eq!( result, CheckResult::NeedsUpdate, "check should detect both duplicate and stale commit" ); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::DuplicateLocalFile, + ), + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::GitStubCommitStale, + ), + ], + ); // A single generate should fix both: remove the duplicate AND update the // stale commit. @@ -2814,7 +2976,8 @@ fn test_stale_git_stub_is_ancestor_error() -> Result<()> { std::env::set_var("FAKE_GIT_FAIL", "is_ancestor"); } - let result = check_apis_up_to_date(env.environment(), &v4)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4)?; // Should report failures: the git_is_ancestor error propagates as an // unfixable GitStubFirstCommitUnknown problem for the non-latest blessed @@ -2824,6 +2987,26 @@ fn test_stale_git_stub_is_ancestor_error() -> Result<()> { CheckResult::Failures, "check should report failures when git_is_ancestor errors" ); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::GitStubFirstCommitUnknown, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::GitStubFirstCommitUnknown, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::GitStubFirstCommitUnknown, + ), + ], + ); Ok(()) } @@ -2884,8 +3067,17 @@ fn test_blessed_version_missing_local_is_fixable_git_stub() -> Result<()> { std::fs::remove_file(env.workspace_root().join(&v3_json_path)) .context("failed to delete blessed v3 file")?; - let result = check_apis_up_to_date(env.environment(), &v3_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v3_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + )], + ); env.generate_documents(&v3_apis)?; @@ -2918,8 +3110,28 @@ fn test_blessed_version_missing_local_is_fixable_git_stub() -> Result<()> { versioned_health_with_v4_trivial_v3_apis(Storage::GitStub)?; // Check should report NeedsUpdate (not Failure). - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "4.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkStale, + ), + ], + ); // Generate should restore v3 as a Git stub (not JSON, since v4 is now // latest and Git stub storage is enabled). @@ -3062,8 +3274,24 @@ fn test_rebase_blessed_version_missing_local_git_stub() -> Result<()> { let rebase_result = env.try_rebase_onto("main")?; assert_eq!(rebase_result, RebaseResult::Clean); - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + ), + ], + ); env.generate_documents(&v4_trivial_apis)?; @@ -3082,8 +3310,24 @@ fn test_merge_blessed_version_missing_local_git_stub() -> Result<()> { let merge_result = env.try_merge_branch("main")?; assert_eq!(merge_result, MergeResult::Clean); - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + ), + ], + ); env.generate_documents(&v4_trivial_apis)?; @@ -3108,8 +3352,24 @@ fn test_jj_rebase_blessed_version_missing_local_git_stub() -> Result<()> { env.jj_new("feature2")?; - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + ), + ], + ); env.generate_documents(&v4_trivial_apis)?; @@ -3133,8 +3393,24 @@ fn test_jj_merge_blessed_version_missing_local_git_stub() -> Result<()> { env.jj_try_merge("feature2", "main", "Merge main into feature2")?; assert_eq!(merge_result, JjMergeResult::Clean); - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + ), + ], + ); env.generate_documents(&v4_trivial_apis)?; diff --git a/crates/integration-tests/tests/integration/lockstep.rs b/crates/integration-tests/tests/integration/lockstep.rs index f617b42..59eb216 100644 --- a/crates/integration-tests/tests/integration/lockstep.rs +++ b/crates/integration-tests/tests/integration/lockstep.rs @@ -9,7 +9,10 @@ use anyhow::Result; use dropshot_api_manager::{ ManagedApiConfig, ManagedApis, - test_util::{CheckResult, check_apis_up_to_date}, + test_util::{ + CheckResult, ProblemKind, ProblemSummary, check_apis_up_to_date, + check_apis_with_summaries, + }, }; use integration_tests::*; use openapiv3::OpenAPI; @@ -155,8 +158,17 @@ fn test_unparseable_conflict_markers() -> Result<()> { "#; env.create_file("documents/health.json", conflict_content)?; - let result = check_apis_up_to_date(env.environment(), &apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ProblemSummary::new( + "health", + "1.0.0", + ProblemKind::LockstepMissingLocal, + )], + ); env.generate_documents(&apis)?; diff --git a/crates/integration-tests/tests/integration/versioned.rs b/crates/integration-tests/tests/integration/versioned.rs index d8ca338..1896954 100644 --- a/crates/integration-tests/tests/integration/versioned.rs +++ b/crates/integration-tests/tests/integration/versioned.rs @@ -8,7 +8,10 @@ use anyhow::{Context, Result}; use camino::Utf8PathBuf; -use dropshot_api_manager::test_util::{CheckResult, check_apis_up_to_date}; +use dropshot_api_manager::test_util::{ + CheckResult, ProblemKind, ProblemSummary, check_apis_up_to_date, + check_apis_with_summaries, +}; use integration_tests::*; use openapiv3::OpenAPI; use semver::Version; @@ -279,8 +282,33 @@ fn blessed_document_lifecycle_impl(env: &TestEnvironment) -> Result<()> { let apis = versioned_health_apis()?; // Initially, APIs should fail the up-to-date check (no documents exist). - let result = check_apis_up_to_date(env.environment(), &apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkMissing, + ), + ], + ); // Generate the documents. env.generate_documents(&apis)?; @@ -321,8 +349,17 @@ fn test_blessed_api_trivial_changes_fail_for_latest() -> Result<()> { // The check should FAIL because the latest version (v3.0.0) has trivial // changes that are semantically equivalent but bytewise different. This // requires a version bump to make the changes visible in PR review. - let result = check_apis_up_to_date(env.environment(), &modified_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &modified_apis)?; assert_eq!(result, CheckResult::Failures); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedLatestVersionBytewiseMismatch, + )], + ); Ok(()) } @@ -375,8 +412,23 @@ fn test_blessed_api_trivial_changes_pass_for_older_versions() -> Result<()> { // version that needs to be generated. Importantly, v1-v3 should pass // despite having trivial changes because they're now older versions // (semantic equality only). - let result = check_apis_up_to_date(env.environment(), &modified_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &modified_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "4.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkStale, + ), + ], + ); // Generate the new v4 document. env.generate_documents(&modified_apis)?; @@ -399,8 +451,52 @@ fn test_mixed_blessed_document_states() -> Result<()> { let combined_apis = multi_versioned_apis()?; // Initially, combined APIs should need update. - let result = check_apis_up_to_date(env.environment(), &combined_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &combined_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "1.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkMissing, + ), + ProblemSummary::new( + "versioned-user", + "1.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-user", + "2.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-user", + "3.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-user", + ProblemKind::LatestLinkMissing, + ), + ], + ); // Generate only health API documents first. let health_apis = versioned_health_apis()?; @@ -408,8 +504,33 @@ fn test_mixed_blessed_document_states() -> Result<()> { env.commit_documents()?; // Combined APIs should still need update (user API missing). - let result = check_apis_up_to_date(env.environment(), &combined_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &combined_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-user", + "1.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-user", + "2.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-user", + "3.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-user", + ProblemKind::LatestLinkMissing, + ), + ], + ); // Generate and commit all APIs documents. env.generate_documents(&combined_apis)?; @@ -459,8 +580,23 @@ fn test_removing_api_version_fails_check() -> Result<()> { let reduced_apis = versioned_health_reduced_apis()?; // The check should result in NeedsUpdate when versions are removed. - let result = check_apis_up_to_date(env.environment(), &reduced_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &reduced_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::LocalSpecFileOrphaned, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkStale, + ), + ], + ); Ok(()) } @@ -483,8 +619,23 @@ fn test_adding_new_api_version_passes_check() -> Result<()> { let expanded_apis = versioned_health_apis()?; // Adding versions should require update (new documents to generate). - let result = check_apis_up_to_date(env.environment(), &expanded_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &expanded_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkStale, + ), + ], + ); // Generate the new versions. env.generate_documents(&expanded_apis)?; @@ -541,8 +692,23 @@ fn test_retiring_latest_blessed_version() -> Result<()> { // This check should return NeedsUpdate because the v3.0.0 document exists // and needs to be removed. - let result = check_apis_up_to_date(env.environment(), &reduced_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &reduced_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::LocalSpecFileOrphaned, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkStale, + ), + ], + ); // Generate documents with the retired version. env.generate_documents(&reduced_apis)?; @@ -589,8 +755,16 @@ fn test_retiring_latest_blessed_version() -> Result<()> { // Delete the latest symlink and ensure that we need to perform updates. env.delete_versioned_latest_symlink("versioned-health")?; - let result = check_apis_up_to_date(env.environment(), &reduced_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &reduced_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkMissing, + )], + ); // Regenerate documents (i.e. the symlink) and retry. env.generate_documents(&reduced_apis)?; @@ -606,8 +780,23 @@ fn test_retiring_latest_blessed_version() -> Result<()> { // Verify we can no longer use the old full API against the new blessed // documents. - let result = check_apis_up_to_date(env.environment(), &full_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &full_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkStale, + ), + ], + ); Ok(()) } @@ -656,8 +845,17 @@ fn test_retiring_older_blessed_version() -> Result<()> { // This check should return NeedsUpdate because the v2.0.0 document exists // and needs to be removed. - let result = check_apis_up_to_date(env.environment(), &skip_middle_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &skip_middle_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::LocalSpecFileOrphaned, + )], + ); // Generate documents with the retired older version. env.generate_documents(&skip_middle_apis)?; @@ -704,8 +902,16 @@ fn test_retiring_older_blessed_version() -> Result<()> { // Delete the latest symlink and ensure that we need to perform updates. env.delete_versioned_latest_symlink("versioned-health")?; - let result = check_apis_up_to_date(env.environment(), &skip_middle_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &skip_middle_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkMissing, + )], + ); // Regenerate documents (i.e. the symlink) and retry. env.generate_documents(&skip_middle_apis)?; @@ -721,8 +927,17 @@ fn test_retiring_older_blessed_version() -> Result<()> { // Verify we can no longer use the old full API against the new blessed // documents. - let result = check_apis_up_to_date(env.environment(), &full_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &full_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "2.0.0", + ProblemKind::LocalVersionMissingLocal, + )], + ); Ok(()) } @@ -770,8 +985,17 @@ fn test_incompatible_blessed_api_change() -> Result<()> { let incompatible_apis = versioned_health_incompat_apis()?; // This check should return Failures. - let result = check_apis_up_to_date(env.environment(), &incompatible_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &incompatible_apis)?; assert_eq!(result, CheckResult::Failures); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionBroken, + )], + ); Ok(()) } @@ -825,8 +1049,17 @@ fn test_blessed_version_extra_local_spec() -> Result<()> { .with_context(|| format!("failed to copy {} to {}", src, dst))?; assert!(dst.exists(), "destination path {dst} exists"); - let result = check_apis_up_to_date(env.environment(), &apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + )], + ); // Regenerating documents should remove the file. env.generate_documents(&apis)?; @@ -1035,12 +1268,20 @@ fn test_malformed_latest_symlink_nonversioned_target() -> Result<()> { // The check should not panic. It should report that updates are needed // (because the "latest" symlink is effectively missing/malformed). - let result = check_apis_up_to_date(env.environment(), &apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &apis)?; assert_eq!( result, CheckResult::NeedsUpdate, "malformed symlink should be detected as needing update" ); + assert_eq!( + summaries, + [ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkMissing, + )], + ); // Generate should fix the symlink. env.generate_documents(&apis)?; @@ -1263,8 +1504,17 @@ fn test_blessed_version_missing_local_is_fixable() -> Result<()> { std::fs::remove_file(env.workspace_root().join(&v3_blessed_path)) .context("failed to delete blessed v3 file")?; - let result = check_apis_up_to_date(env.environment(), &v3_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v3_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + )], + ); env.generate_documents(&v3_apis)?; @@ -1294,8 +1544,28 @@ fn test_blessed_version_missing_local_is_fixable() -> Result<()> { versioned_health_with_v4_trivial_v3_apis(Storage::Concrete)?; // Check should report NeedsUpdate (not Failure). - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "4.0.0", + ProblemKind::LocalVersionMissingLocal, + ), + ProblemSummary::for_api( + "versioned-health", + ProblemKind::LatestLinkStale, + ), + ], + ); // Generate should restore the blessed v3 and create a v4. env.generate_documents(&v4_trivial_apis)?; @@ -1411,8 +1681,24 @@ fn test_rebase_blessed_version_missing_local() -> Result<()> { let rebase_result = env.try_rebase_onto("main")?; assert_eq!(rebase_result, RebaseResult::Clean); - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + ), + ], + ); env.generate_documents(&v4_trivial_apis)?; @@ -1431,8 +1717,24 @@ fn test_merge_blessed_version_missing_local() -> Result<()> { let merge_result = env.try_merge_branch("main")?; assert_eq!(merge_result, MergeResult::Clean); - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + ), + ], + ); env.generate_documents(&v4_trivial_apis)?; @@ -1457,8 +1759,24 @@ fn test_jj_rebase_blessed_version_missing_local() -> Result<()> { env.jj_new("feature2")?; - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + ), + ], + ); env.generate_documents(&v4_trivial_apis)?; @@ -1482,8 +1800,24 @@ fn test_jj_merge_blessed_version_missing_local() -> Result<()> { env.jj_try_merge("feature2", "main", "Merge main into feature2")?; assert_eq!(merge_result, JjMergeResult::Clean); - let result = check_apis_up_to_date(env.environment(), &v4_trivial_apis)?; + let (result, summaries) = + check_apis_with_summaries(env.environment(), &v4_trivial_apis)?; assert_eq!(result, CheckResult::NeedsUpdate); + assert_eq!( + summaries, + [ + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionMissingLocal, + ), + ProblemSummary::new( + "versioned-health", + "3.0.0", + ProblemKind::BlessedVersionExtraLocalSpec, + ), + ], + ); env.generate_documents(&v4_trivial_apis)?; From 5b959b720b19db3e4a1f657c00556457b2b1ad34 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 13 Mar 2026 11:16:02 -0700 Subject: [PATCH 2/2] fix a directory ordering issue Created using spr 1.3.6-beta.1 --- .../src/spec_files_local.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/crates/dropshot-api-manager/src/spec_files_local.rs b/crates/dropshot-api-manager/src/spec_files_local.rs index a4dbbfb..0d82764 100644 --- a/crates/dropshot-api-manager/src/spec_files_local.rs +++ b/crates/dropshot-api-manager/src/spec_files_local.rs @@ -299,17 +299,22 @@ fn discover_local_entries( let top_iter = dir.read_dir_utf8().with_context(|| format!("readdir {:?}", dir))?; + // Collect and sort by file name to ensure deterministic ordering + // across platforms and filesystems. + let mut top_entries = Vec::new(); for maybe_entry in top_iter { - let entry = match maybe_entry { - Ok(e) => e, + match maybe_entry { + Ok(e) => top_entries.push(e), Err(error) => { entries.push(LocalDiscoveredEntry::Error( anyhow!(error).context(format!("readdir {:?} entry", dir)), )); - continue; } - }; + } + } + top_entries.sort_by(|a, b| a.file_name().cmp(b.file_name())); + for entry in top_entries { let path = entry.path().to_owned(); let file_name = entry.file_name().to_owned(); let file_type = match entry.file_type() { @@ -345,7 +350,7 @@ fn discover_versioned_directory( path: &Utf8Path, dir_basename: &str, ) { - let sub_entries = match path + let mut sub_entries = match path .read_dir_utf8() .and_then(|iter| iter.collect::, _>>()) { @@ -357,6 +362,9 @@ fn discover_versioned_directory( return; } }; + // Sort by file name to ensure deterministic ordering across + // platforms and filesystems. + sub_entries.sort_by(|a, b| a.file_name().cmp(b.file_name())); // Construct a temporary ApiIdent so we can use its canonical // symlink-detection method. This ident is not validated against the