Skip to content

Commit

Permalink
test(iroh-cli): reduce flakyness of cli_provide_file_resume (#2563)
Browse files Browse the repository at this point in the history
## Description

In windows there is no way to copy a file being accessed by another
process in an accessible way. This is what makes this test fail since it
attempt to copy the blobs.db folder a couple of times while the iroh
instance that handles is running.

The change is simple: shutdown the provider, copy the files, re-start
the provider. From my perspective, this does not affect what the test is
attempting to assert.

Now, since this includes re-starting the iroh instance that provides the
files several times, and we match on output, sometimes we can get weird
logs related to shutdown. One of those (and the only one I have seen so
far) is for a netcheck report and didn't finish on time. Instead of
logging this in the reportgen actor, the error is bubbled up to be
handled by the netcheck actor (which will have shutdown by then) thus
reducing noise and allowing for better error handling in the future

## Breaking Changes

n/a

## Notes & open questions

n/a

## Change checklist

- [x] Self-review.
- [ ] ~~Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.~~
- [x] Tests if relevant.
- [ ] ~~All breaking changes documented.~~
  • Loading branch information
divagant-martian authored Jul 29, 2024
1 parent 14fccee commit f085e63
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 11 deletions.
15 changes: 11 additions & 4 deletions iroh-cli/tests/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,8 @@ fn cli_provide_tree_resume() -> Result<()> {
Ok(())
}

#[test]
#[ignore = "flaky"]
#[test]
fn cli_provide_file_resume() -> Result<()> {
use iroh::blobs::store::fs::test_support::{make_partial, MakePartialResult};

Expand All @@ -229,11 +229,14 @@ fn cli_provide_file_resume() -> Result<()> {
let src_iroh_data_dir = tmp.join("src_iroh_data_dir");
let file = src.join("file");
let hash = make_rand_file(100000, &file)?;
// leave the provider running for the entire test
// import the files into an ephemeral iroh to use the generated blobs db in tests
let provider = make_provider_in(&src_iroh_data_dir, Input::Path(file.clone()), false)?;
let count = count_input_files(&src);
let ticket = match_provide_output(&provider, count, BlobOrCollection::Blob)?;
drop(provider);

{
let provider = make_provider_in(&src_iroh_data_dir, Input::Path(file.clone()), false)?;
println!("first test - empty work dir");
let get_iroh_data_dir = tmp.join("get_iroh_data_dir_01");
let get_output = run_get_cmd(&get_iroh_data_dir, &ticket, Some(tgt.clone()))?;
Expand All @@ -242,25 +245,29 @@ fn cli_provide_file_resume() -> Result<()> {
assert_eq!(Hash::new(std::fs::read(&tgt)?), hash);
// compare_files(&src, &tgt)?;
std::fs::remove_file(&tgt)?;
drop(provider);
}

// second test - full work dir
{
println!("second test - full work dir");
let get_iroh_data_dir = tmp.join("get_iroh_data_dir_02");
copy_blob_dirs(&src_iroh_data_dir, &get_iroh_data_dir)?;
let provider = make_provider_in(&src_iroh_data_dir, Input::Path(file.clone()), false)?;
let get_output = run_get_cmd(&get_iroh_data_dir, &ticket, Some(tgt.clone()))?;
let matches = explicit_matches(match_get_stderr(get_output.stderr)?);
assert_eq!(matches, vec!["0 B"]);
assert_eq!(Hash::new(std::fs::read(&tgt)?), hash);
std::fs::remove_file(&tgt)?;
drop(provider);
}

// third test - partial work dir - truncate some large files
{
println!("fourth test - partial work dir - truncate some large files");
let get_iroh_data_dir = tmp.join("get_iroh_data_dir_04");
copy_blob_dirs(&src_iroh_data_dir, &get_iroh_data_dir)?;
let provider = make_provider_in(&src_iroh_data_dir, Input::Path(file.clone()), false)?;
make_partial(&get_iroh_data_dir, |_hash, _size| {
MakePartialResult::Truncate(1024 * 32)
})?;
Expand All @@ -269,8 +276,8 @@ fn cli_provide_file_resume() -> Result<()> {
assert_eq!(matches, vec!["65.98 KiB"]);
assert_eq!(Hash::new(std::fs::read(&tgt)?), hash);
std::fs::remove_file(&tgt)?;
drop(provider);
}
drop(provider);
Ok(())
}

Expand Down Expand Up @@ -619,7 +626,7 @@ fn iroh_bin() -> &'static str {
env!("CARGO_BIN_EXE_iroh")
}

/// Makes a provider process with it's home directory in `iroh_data_dir`.
/// Makes a provider process with its home directory in `iroh_data_dir`.
fn make_provider_in(iroh_data_dir: &Path, input: Input, wrap: bool) -> Result<ReaderHandle> {
let mut args = vec!["--metrics-port", "disabled", "start"];
if wrap {
Expand Down
10 changes: 5 additions & 5 deletions iroh-net/src/netcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ pub(crate) enum Message {
/// A report produced by the [`reportgen`] actor.
ReportReady { report: Box<Report> },
/// The [`reportgen`] actor failed to produce a report.
ReportAborted,
ReportAborted { err: anyhow::Error },
/// An incoming STUN packet to parse.
StunPacket {
/// The raw UDP payload.
Expand Down Expand Up @@ -458,8 +458,8 @@ impl Actor {
Message::ReportReady { report } => {
self.handle_report_ready(report);
}
Message::ReportAborted => {
self.handle_report_aborted();
Message::ReportAborted { err } => {
self.handle_report_aborted(err);
}
Message::StunPacket { payload, from_addr } => {
self.handle_stun_packet(&payload, from_addr);
Expand Down Expand Up @@ -547,10 +547,10 @@ impl Actor {
}
}

fn handle_report_aborted(&mut self) {
fn handle_report_aborted(&mut self, err: anyhow::Error) {
self.in_flight_stun_requests.clear();
if let Some(ReportRun { report_tx, .. }) = self.current_report_run.take() {
report_tx.send(Err(anyhow!("report aborted"))).ok();
report_tx.send(Err(err.context("report aborted"))).ok();
}
}

Expand Down
3 changes: 1 addition & 2 deletions iroh-net/src/netcheck/reportgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,8 @@ impl Actor {
match self.run_inner().await {
Ok(_) => debug!("reportgen actor finished"),
Err(err) => {
error!("reportgen actor failed: {err:#}");
self.netcheck
.send(netcheck::Message::ReportAborted)
.send(netcheck::Message::ReportAborted { err })
.await
.ok();
}
Expand Down

0 comments on commit f085e63

Please sign in to comment.