[integration-tests] assert on problem list (#82)

Previously we would only assert on the final status in our tests. But we
care about which APIs had problems detected with them.

Exposing the full `Problem` type is very complex, so we define a
`ProblemKind` enum that's just the discriminant.

Also fix a `read_dir` ordering issue detected in CI.

Author: sunshowers

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<CheckResult> {
+    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<ProblemSummary>)> {
     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))
 }

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<semver::Version>,
+    /// 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<Note>,
-    non_version_problems: Vec<Problem<'a>>,
+    non_version_problems: Vec<(ApiIdent, Option<semver::Version>, Problem<'a>)>,
     api_results: BTreeMap<ApiIdent, ApiResolved<'a>>,
     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<Problem<'_>> =
-            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<semver::Version>,
+            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<Item = &Problem<'a>> + '_ {
-        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<ProblemSummary> {
+        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> {

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::<Result<Vec<_>, _>>())
     {
@@ -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