Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: break refresh opt into bless and update #261

Merged
merged 7 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
368 changes: 149 additions & 219 deletions Arena.toml

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ We also appreciate feedback on our documentation. Feel free to look over any of

[Gauntlet](https://github.com/stjude-rust-labs/wdl/tree/main/gauntlet) is the main driver of our CI. Take a look at the file [`Gauntlet.toml`](https://github.com/stjude-rust-labs/wdl/blob/main/Gauntlet.toml). The entries at the top are all GitHub repositories of WDL code. The remaining entries are diagnostics emitted while analyzing those repositories. These should remain relatively static between PRs, and any change in emitted diagnostics should be reviewed carefully.

In order to turn the Gauntlet CI green, run `cargo run --release --bin gauntlet -- --refresh`. The `--refresh` flag will save any changes to the `Gauntlet.toml` file. This should then be committed and included in your PR.
In order to turn the Gauntlet CI green, run `cargo run --release --bin gauntlet -- --bless`. The `--bless` flag will save any changes to the `Gauntlet.toml` file. This should then be committed and included in your PR.

### What is arena?

Arena is the alternate run mode of `gauntlet`. [`Arena.toml`](https://github.com/stjude-rust-labs/wdl/blob/main/Arena.toml) is very similar to `Gauntlet.toml`, except it has fewer repository entries and instead of analysis diagnostics it contains only lint diagnostics (which are not included in `Gauntlet.toml`).

In order to turn the Arena CI green, run `cargo run --release --bin gauntlet -- --arena --refresh`. The `--refresh` flag (in conjunction with the `--arena` flag) will save any changes to the `Arena.toml` file. This should then be committed and included in your PR.
In order to turn the Arena CI green, run `cargo run --release --bin gauntlet -- --arena --bless`. The `--bless` flag (in conjunction with the `--arena` flag) will save any changes to the `Arena.toml` file. This should then be committed and included in your PR.

### The CI has turned red. How do I make it green again?

Expand All @@ -61,9 +61,9 @@ There are a handful of reasons the CI may have turned red. Try the following fix
- `cargo clippy --all-features` and then fix any warnings emitted
- `BLESS=1 cargo test --all-features` to "bless" any test changes
- Please review any changes this causes to make sure they seem right!
- `cargo run --release --bin gauntlet -- --refresh`
- `cargo run --release --bin gauntlet -- --bless`
- see the `What is gauntlet?` question for more information
- `cargo run --release --bin gauntlet -- --refresh --arena`
- `cargo run --release --bin gauntlet -- --bless --arena`
- see the `What is arena?` question for more information
- `rustup update` to update your local toolchains

Expand Down
1 change: 1 addition & 0 deletions gauntlet/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Use `tracing` events instead of the `log` crate ([#172](https://github.com/stjude-rust-labs/wdl/pull/172))
* Changed name from `wdl-gauntlet` to just `gauntlet`
* Set `publish = false` in `Cargo.toml`
* Break `refresh` option into `bless` and `update` flags ([#261](https://github.com/stjude-rust-labs/wdl/pull/261))

## 0.5.0 - 08-22-2024

Expand Down
27 changes: 16 additions & 11 deletions gauntlet/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,17 @@ pub struct Args {
#[arg(short, long)]
pub quiet: bool,

/// Overwrites the configuration file with new expected diagnostics and the
/// latest commit hashes. This will create temporary shallow clones of every
/// test repository. Normally, there is only one repository on disk at a
/// time. The difference in disk space usage should be negligible.
#[arg(long)]
pub refresh: bool,
/// Overwrites the configuration file with new expected diagnostics.
///
/// Mutually exclusive with `--update`.
#[arg(long, group = "action")]
pub bless: bool,
a-frantz marked this conversation as resolved.
Show resolved Hide resolved

/// Updates the commit hashes for all repositories.
///
/// Mutually exclusive with `--bless`.
#[arg(long, group = "action")]
pub update: bool,

/// Displays warnings as part of the report output.
#[arg(long)]
Expand Down Expand Up @@ -137,8 +142,8 @@ pub async fn gauntlet(args: Args) -> Result<()> {

let mut work_dir = WorkDir::default();

if args.refresh {
info!("refreshing repository commit hashes.");
if args.update {
info!("updating repository commit hashes.");
config.inner_mut().update_repositories(work_dir.root());
}

Expand Down Expand Up @@ -356,7 +361,7 @@ pub async fn gauntlet(args: Args) -> Result<()> {
};

// Don't bother rebuilding the diagnostics
if !args.refresh {
if !args.bless && !args.update {
continue;
}

Expand All @@ -380,7 +385,7 @@ pub async fn gauntlet(args: Args) -> Result<()> {

println!("\nTotal analysis time: {total_time:?}");

if args.refresh {
if args.bless || args.update {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only fire on --bless

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we need to save the new diagnostics (and new hashes) back to the relevant config file when --update is passed. Otherwise --update would be a no-op that fails whenever something changed.

info!("adding {unexpected} new expected diagnostics.");
info!("removing {missing} outdated expected diagnostics.");

Expand All @@ -391,7 +396,7 @@ pub async fn gauntlet(args: Args) -> Result<()> {
println!(
"\n{}\n",
"missing but expected diagnostics remain: you should remove these from your \
configuration file or run the command with the `--refresh` option!"
configuration file or run the command with the `--bless` option!"
.red()
.bold()
);
Expand Down
26 changes: 12 additions & 14 deletions wdl-engine/src/eval/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ impl<'a, C: EvaluationContext> ExprEvaluator<'a, C> {
/// Used to translate an expression evaluation context to an expression
/// type evaluation context.
struct TypeContext<'a, C: EvaluationContext>(&'a mut C);
impl<'a, C: EvaluationContext> wdl_analysis::types::v1::EvaluationContext for TypeContext<'a, C> {
impl<C: EvaluationContext> wdl_analysis::types::v1::EvaluationContext for TypeContext<'_, C> {
fn version(&self) -> SupportedVersion {
self.0.version()
}
Expand Down Expand Up @@ -1093,21 +1093,19 @@ impl<'a, C: EvaluationContext> ExprEvaluator<'a, C> {
match f.param_min_max(self.context.version()) {
Some((_, max)) => {
assert!(max <= MAX_PARAMETERS);
return Err(too_many_arguments(
Err(too_many_arguments(
target.as_str(),
target.span(),
max,
count,
expr.arguments().skip(max).map(|e| e.span()),
));
}
None => {
return Err(unsupported_function(
f.minimum_version(),
target.as_str(),
target.span(),
));
))
}
None => Err(unsupported_function(
f.minimum_version(),
target.as_str(),
target.span(),
)),
}
}
}
Expand Down Expand Up @@ -1350,7 +1348,7 @@ pub(crate) mod test {
self.env
.scope()
.lookup(name.as_str())
.map(|v| v.clone())
.cloned()
.ok_or_else(|| unknown_name(name.as_str(), name.span()))
}

Expand Down Expand Up @@ -1417,7 +1415,7 @@ pub(crate) mod test {
);
let output = parser.finish();
assert_eq!(
output.diagnostics.iter().next(),
output.diagnostics.first(),
None,
"the provided WDL source failed to parse"
);
Expand Down Expand Up @@ -1496,10 +1494,10 @@ pub(crate) mod test {
approx::assert_relative_eq!(value.unwrap_float(), -12345.6789);

let value = eval_v1_expr(&mut env, V1::Two, "1.7976931348623157E+308").unwrap();
approx::assert_relative_eq!(value.unwrap_float(), 1.7976931348623157E+308);
approx::assert_relative_eq!(value.unwrap_float(), 1.797_693_134_862_315_7E308);

let value = eval_v1_expr(&mut env, V1::Two, "-1.7976931348623157E+308").unwrap();
approx::assert_relative_eq!(value.unwrap_float(), -1.7976931348623157E+308);
approx::assert_relative_eq!(value.unwrap_float(), -1.797_693_134_862_315_7E308);

let diagnostic =
eval_v1_expr(&mut env, V1::Two, "2.7976931348623157E+308").expect_err("should fail");
Expand Down
4 changes: 2 additions & 2 deletions wdl-engine/tests/inputs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ fn run_test(test: &Path, result: AnalysisResult) -> Result<()> {
.parse_result()
.root()
.map(|n| SyntaxNode::new_root(n.clone()).text().to_string())
.unwrap_or(String::new());
.unwrap_or_default();
let file = SimpleFile::new(&path, &source);

term::emit(
Expand Down Expand Up @@ -221,7 +221,7 @@ async fn main() {
});

let result = results.next().expect("should have a result");
if !results.next().is_none() {
if results.next().is_some() {
println!("test {test_name} ... {failed}", failed = "failed".red());
errors.push((
test_name,
Expand Down
4 changes: 2 additions & 2 deletions wdl-grammar/src/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ pub enum PreambleToken {
/// Asserts that PreambleToken can fit in a TokenSet.
const _: () = assert!(PreambleToken::MAX as u8 <= 128);

impl<'a> ParserToken<'a> for PreambleToken {
impl ParserToken<'_> for PreambleToken {
fn into_syntax(self) -> SyntaxKind {
match self {
Self::Whitespace => SyntaxKind::Whitespace,
Expand Down Expand Up @@ -170,7 +170,7 @@ pub enum VersionStatementToken {
/// Asserts that VersionStatementToken can fit in a TokenSet.
const _: () = assert!(VersionStatementToken::MAX as u8 <= 128);

impl<'a> ParserToken<'a> for VersionStatementToken {
impl ParserToken<'_> for VersionStatementToken {
fn into_syntax(self) -> SyntaxKind {
match self {
Self::Whitespace => SyntaxKind::Whitespace,
Expand Down
4 changes: 2 additions & 2 deletions wdl-lint/src/rules/blank_lines_between_elements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,10 +665,10 @@ fn skip_preceding_comments(syntax: &SyntaxNode) -> NodeOrToken<SyntaxNode, Synta
prev = cur.prev_token()
}

return preceding_comments.last().map_or_else(
preceding_comments.last().map_or_else(
|| SyntaxElement::from(syntax.clone()),
|c| SyntaxElement::from(c.clone()),
);
)
}

/// Is first body element?
Expand Down
4 changes: 2 additions & 2 deletions wdl-lint/src/rules/nonmatching_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ pub struct NonmatchingOutputRule<'a> {
prior_objects: Vec<String>,
}

impl<'a> Rule for NonmatchingOutputRule<'a> {
impl Rule for NonmatchingOutputRule<'_> {
fn id(&self) -> &'static str {
ID
}
Expand Down Expand Up @@ -233,7 +233,7 @@ fn handle_meta_outputs_and_reset(
rule.meta_outputs_keys.clear();
}

impl<'a> Visitor for NonmatchingOutputRule<'a> {
impl Visitor for NonmatchingOutputRule<'_> {
type State = Diagnostics;

fn document(
Expand Down
2 changes: 1 addition & 1 deletion wdl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ mod test {
#[test]
fn reserved_rule_ids() {
let rules: HashSet<_> = wdl_analysis::rules().iter().map(|r| r.id()).collect();
let reserved: HashSet<_> = wdl_lint::RESERVED_RULE_IDS.iter().map(|id| *id).collect();
let reserved: HashSet<_> = wdl_lint::RESERVED_RULE_IDS.iter().copied().collect();

for id in &reserved {
if !rules.contains(id) {
Expand Down
Loading