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

ci: improve test resilience and run style checks within cargo nextest #9608

Merged
merged 17 commits into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from 15 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
26 changes: 26 additions & 0 deletions .config/nextest.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,29 @@ slow-timeout = { period = "60s", terminate-after = 2, grace-period = "0s" }
[[profile.default.overrides]]
filter = 'test(test_full_estimator)'
slow-timeout = { period = "10m", terminate-after = 3 }
threads-required = 4

[[profile.default.overrides]]
filter = 'package(style-tests)'
slow-timeout = { period = "120s", terminate-after = 5 }
threads-required = 4

# Unfortunately no support for inheriting profiles yet:
# https://github.com/nextest-rs/nextest/issues/387
[profile.ci]
nagisa marked this conversation as resolved.
Show resolved Hide resolved
slow-timeout = { period = "120s", terminate-after = 5 }
# Try a few times before failing the whole test suite on a potentially spurious tests.
# The hope is that people will fix the spurious tests as they encounter them locally...
retries = { backoff = "fixed", count = 3, delay = "1s" }
failure-output = "final"
fail-fast = false

[[profile.ci.overrides]]
filter = 'test(test_full_estimator)'
slow-timeout = { period = "10m", terminate-after = 3 }
threads-required = 4

[[profile.ci.overrides]]
filter = 'package(style-tests)'
slow-timeout = { period = "120s", terminate-after = 5 }
threads-required = 4
4 changes: 4 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ members = [
"test-utils/runtime-tester/fuzz",
"test-utils/store-validator",
"test-utils/testlib",
"test-utils/style",
"tools/database",
"tools/chainsync-loadtest",
"tools/delay-detector",
Expand Down
4 changes: 2 additions & 2 deletions core/o11y/src/io_tracer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ impl<S: Subscriber + for<'span> LookupSpan<'span>> Layer<S> for IoTraceLayer {
}
}

#[allow(clippy::integer_arithmetic)]
#[allow(clippy::arithmetic_side_effects)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also update docs/practices/style.md

285:`clippy::integer_arithmetic` lints.

fn on_exit(&self, id: &span::Id, ctx: tracing_subscriber::layer::Context<'_, S>) {
// When the span exits, produce one line for the span itself that
// includes key=value pairs from `SpanInfo`. Then also add indentation
Expand Down Expand Up @@ -311,7 +311,7 @@ impl tracing::field::Visit for IoEventVisitor {
}

impl tracing::field::Visit for SpanInfo {
#[allow(clippy::integer_arithmetic)]
#[allow(clippy::arithmetic_side_effects)]
fn record_str(&mut self, field: &tracing::field::Field, value: &str) {
// "count" is a special field, everything else are key values pairs.
if field.name() == "counter" {
Expand Down
8 changes: 4 additions & 4 deletions core/o11y/src/testonly.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,24 @@ fn setup_subscriber_from_filter(mut env_filter: EnvFilter) {
}

pub fn init_test_logger() {
let env_filter = EnvFilter::new("tokio_reactor=info,tokio_core=info,hyper=info,debug");
let env_filter = EnvFilter::new("cranelift=warn,wasmtime=warn,h2=warn,tower=warn,trust_dns=warn,tokio_reactor=info,tokio_core=info,hyper=info,debug");
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

setup_subscriber_from_filter(env_filter);
}

pub fn init_test_logger_allow_panic() {
let env_filter = EnvFilter::new("tokio_reactor=info,tokio_core=info,hyper=info,debug");
let env_filter = EnvFilter::new("cranelift=warn,wasmtime=warn,h2=warn,tower=warn,trust_dns=warn,tokio_reactor=info,tokio_core=info,hyper=info,debug");
setup_subscriber_from_filter(env_filter);
}

pub fn init_test_module_logger(module: &str) {
let env_filter =
EnvFilter::new("tokio_reactor=info,tokio_core=info,hyper=info,cranelift_wasm=warn,info")
EnvFilter::new("cranelift=warn,wasmtime=warn,h2=warn,tower=warn,trust_dns=warn,tokio_reactor=info,tokio_core=info,hyper=info,cranelift_wasm=warn,info")
.add_directive(format!("{}=info", module).parse().unwrap());
setup_subscriber_from_filter(env_filter);
}

pub fn init_integration_logger() {
let env_filter = EnvFilter::new("actix_web=warn,info");
let env_filter = EnvFilter::new("cranelift=warn,wasmtime=warn,actix_web=warn,info");
setup_subscriber_from_filter(env_filter);
}

Expand Down
6 changes: 3 additions & 3 deletions integration-tests/src/tests/client/epoch_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ fn generate_transactions(last_hash: &CryptoHash, h: BlockHeight) -> Vec<SignedTr
vec![Action::DeployContract(DeployContractAction {
code: near_test_contracts::rs_contract().to_vec(),
})],
last_hash.clone(),
*last_hash,
));
}

Expand All @@ -40,7 +40,7 @@ fn generate_transactions(last_hash: &CryptoHash, h: BlockHeight) -> Vec<SignedTr
gas: 100_000_000_000_000,
deposit: 0,
}))],
last_hash.clone(),
*last_hash,
));
}

Expand All @@ -51,7 +51,7 @@ fn generate_transactions(last_hash: &CryptoHash, h: BlockHeight) -> Vec<SignedTr
"test1".parse().unwrap(),
&signer,
1,
last_hash.clone(),
*last_hash,
));
}
txs
Expand Down
1 change: 0 additions & 1 deletion runtime/near-vm-runner/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ pub(crate) mod test_builder;
mod ts_contract;
mod wasm_validation;

#[cfg(all(feature = "near_vm", target_arch = "x86_64"))]
use crate::config::ContractPrepareVersion;
use crate::logic::{Config, VMContext};
use crate::vm_kind::VMKind;
Expand Down
4 changes: 3 additions & 1 deletion runtime/near-vm-runner/src/tests/fuzzers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ fn current_vm_does_not_crash() {
}

#[test]
#[cfg_attr(not(target_arch = "x86_64"), ignore)]
fn wasmer2_and_wasmtime_agree() {
check!().for_each(|data: &[u8]| {
let module = ArbitraryModule::arbitrary(&mut arbitrary::Unstructured::new(data));
Expand All @@ -174,6 +175,7 @@ fn wasmer2_and_wasmtime_agree() {
}

#[test]
#[cfg_attr(not(all(feature = "near_vm", target_arch = "x86_64")), ignore)]
fn near_vm_and_wasmtime_agree() {
check!().for_each(|data: &[u8]| {
let module = ArbitraryModule::arbitrary(&mut arbitrary::Unstructured::new(data));
Expand All @@ -188,8 +190,8 @@ fn near_vm_and_wasmtime_agree() {
});
}

#[cfg(all(feature = "near_vm", target_arch = "x86_64"))]
#[test]
#[cfg(all(feature = "near_vm", target_arch = "x86_64"))]
fn near_vm_is_reproducible() {
use crate::near_vm_runner::NearVM;
use near_primitives::hash::CryptoHash;
Expand Down
8 changes: 7 additions & 1 deletion runtime/near-vm-runner/src/tests/test_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,18 @@ pub(crate) fn test_builder() -> TestBuilder {
view_config: None,
output_data_receivers: vec![],
};
let mut skip = HashSet::new();
if cfg!(not(target_arch = "x86_64")) {
skip.insert(VMKind::Wasmer0);
skip.insert(VMKind::Wasmer2);
skip.insert(VMKind::NearVm);
}
TestBuilder {
code: ContractCode::new(Vec::new(), None),
context,
method: "main".to_string(),
protocol_versions: vec![u32::MAX],
skip: HashSet::new(),
skip,
opaque_error: false,
opaque_outcome: false,
}
Expand Down
2 changes: 2 additions & 0 deletions runtime/near-vm/test-api/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
#![cfg(target_arch = "x86_64")]

mod sys;
pub use sys::*;
2 changes: 1 addition & 1 deletion runtime/near-vm/tests/compilers/main.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![cfg(target_arch = "x86_64")]
//! This test suite does all the tests that involve any compiler
//! implementation, such as: singlepass.
#[macro_use]
extern crate near_vm_compiler_test_derive;

Expand Down
2 changes: 1 addition & 1 deletion runtime/near-vm/wast/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! Implementation of the WAST text format for wasmer.
#![cfg(target_arch = "x86_64")]
#![deny(missing_docs, trivial_numeric_casts, unused_extern_crates)]
#![warn(unused_import_braces)]
#![deny(unstable_features)]
Expand Down
2 changes: 1 addition & 1 deletion rust-toolchain.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
# Individual crates in the workspace may support a lower version, as indicated by `rust-version` field in each crate's `Cargo.toml`.
# The version specified below, should be at least as high as the maximum `rust-version` within the workspace.
channel = "1.72.0"
components = [ "rustfmt" ]
components = [ "rustfmt", "clippy" ]
targets = [ "wasm32-unknown-unknown" ]
7 changes: 7 additions & 0 deletions test-utils/style/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "style-tests"
version = "0.0.0"
edition = "2021"
publish = false
description = "An entry point for miscelaneous (stylistic) workspace-wide tests"
authors = ["Near Inc <hello@nearprotocol.com>"]
82 changes: 82 additions & 0 deletions test-utils/style/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#![cfg(test)]
use std::{
ffi::{OsStr, OsString},
path::PathBuf,
process::Command,
};

/// Add common cargo arguments for tests run by this code.
fn cargo_env(cmd: &mut Command) {
// Set the working directory to the project root, rather than using whatever default nextest
// gives us.
let style_root = std::env::var_os("CARGO_MANIFEST_DIR").unwrap_or(OsString::from("./"));
let wp_root: PathBuf = [&style_root, OsStr::new(".."), OsStr::new("..")].into_iter().collect();
cmd.current_dir(&wp_root);

// Use a different target directory to avoid invalidating any cache after tests are run (so
// that running `cargo nextest` twice does not rebuild half of the workspace on the 2nd
// rebuild. Unfortunately cargo itself does not readily expose this information to us, so we
// have to guess a little as to where this directory might end up.
//
// NB: We aren't using a temporary directory proper here in order to *allow* keeping cache
// between individual `clippy` runs and such.
let target_dir: PathBuf =
[wp_root.as_os_str(), OsStr::new("target"), OsStr::new("style")].into_iter().collect();
cmd.env("CARGO_TARGET_DIR", target_dir.as_path());
}

fn ensure_success(mut cmd: std::process::Command) {
println!("Running {:?}", cmd);
match cmd.status() {
Err(e) => {
panic!("Could not spawn the command: {e}")
}
Ok(out) if !out.success() => panic!("exit code {:?}", out),
Ok(_) => {}
}
}

#[test]
fn rustfmt() {
let cargo = std::env::var_os("CARGO").unwrap_or(OsString::from("cargo"));
let mut cmd = Command::new(cargo);
cargo_env(&mut cmd);
cmd.args(&["fmt", "--", "--check"]);
ensure_success(cmd);
}

#[test]
fn clippy() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function duplicates scripts/run_clippy.sh. I see that you plan to delete that file in #9290.
Should be fine, but maybe add a TODO to remove the duplicated files.

let cargo = std::env::var_os("CARGO").unwrap_or(OsString::from("cargo"));
let mut cmd = Command::new(cargo);
cargo_env(&mut cmd);
cmd.args(&["clippy", "--all-targets", "--all-features", "--"]);
cmd.args(&[
"-Aclippy::all",
"-Dclippy::clone_on_copy",
"-Dclippy::correctness",
"-Dclippy::derivable_impls",
"-Dclippy::redundant_clone",
"-Dclippy::suspicious",
"-Dclippy::len_zero",
]);
ensure_success(cmd);
}

#[test]
fn deny() {
let cargo = std::env::var_os("CARGO").unwrap_or(OsString::from("cargo"));
let mut cmd = Command::new(cargo);
cargo_env(&mut cmd);
cmd.args(&["deny", "--all-features", "check", "bans"]);
ensure_success(cmd);
}

#[test]
fn themis() {
let cargo = std::env::var_os("CARGO").unwrap_or(OsString::from("cargo"));
let mut cmd = Command::new(cargo);
cargo_env(&mut cmd);
cmd.args(&["run", "-p", "themis"]);
ensure_success(cmd);
}