fix: code quality and test integrity - #3
Closed
tomdavidson wants to merge 1 commit into
Closed
Conversation
- Fix sort in linter::lint() to sort by file path then offset (was offset-only, causing diagnostics from different files to interleave incorrectly) - Fix is_valid_identifier() to reject leading hyphen in job IDs (GitHub Actions requires job IDs to start with a letter or underscore) - Remove dead `job_count` variable and its `let _ = job_count` suppression in check_circular_needs() - Move fuzz_regression module from pub to pub(crate) so test infrastructure does not leak into the public library API - Document ignore_patterns as substring matching (not glob) in LintConfig - Add test for leading-hyphen job ID validation - Add severity assertions to no_jobs test (must be Error, not Warning) - Replace magic-number aggregate test (>= 15) with explicit rule enumeration
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the code quality and test integrity issues identified in review.
Bug fixes
linter::lint()— sort was broken across multiple filesThe comparator sorted by
span.offset()alone, which interleaved diagnostics from different files when their byte offsets collided. Fixed to sort byfile_path()first, thenspan.offset().is_valid_identifier()— leading hyphen was acceptedGitHub Actions job IDs must start with a letter or underscore; a leading
-is invalid. The old check included|| c == '-'for the first character. Removed. Two new tests cover this:leading_hyphen_job_id_invalidandhyphen_in_middle_of_job_id_valid.Code quality
fuzz_regressionremoved from public APIlib.rsexportedfuzz_regressionaspub, leaking test infrastructure into the library's public surface. Changed topub(crate).Dead
job_countvariable removedcheck_circular_needscomputedlet job_count = workflow.jobs.len()and then immediately suppressed the warning withlet _ = job_count. Both lines removed.ignore_patternsdocumented as substring matchingAdded doc comment to
LintConfig.ignore_patternsmaking it explicit that these are plain substring matches, not globs. This prevents users from writingnode_modules/**and expecting glob semantics.Test integrity
Magic-number aggregate test replaced
invalid_directory_produces_findingsasserteddiagnostics.len() >= 15. A rule regression producing 16 diagnostics instead of the expected 18 would silently pass. Replaced withinvalid_directory_has_expected_ruleswhich enumerates all 18 expected rule names explicitly.Severity helper + assertion added
New
has_rule_with_severity()helper in the integration test harness. Theno_jobs_detectedtest now asserts the diagnostic isSeverity::Error, catching any accidental demotion toWarning. The unit test forno_jobsinjobs.rsdoes the same.What is NOT in this PR
The following issues from the review require more context or separate work:
deny.tomlconfiguration (licenses + advisories) — needs policy decisiondescription/keywords/categoriesmetadata — needs copy decision#![warn(missing_docs)]enforcement — worthwhile but large surface areainstaadded as dev-dependencyignore_patterns— needsglobsetand a semver-visible behaviour change