Page MenuHomePhabricator

[services] Rust Integration - Backup - c++ - Fix wrong order of attachment holders in tests
ClosedPublic

Authored by karol on Sep 6 2022, 7:23 AM.
Tags
None
Referenced Files
F1748060: D5074.id16377.diff
Mon, May 13, 5:23 PM
F1748059: D5074.id.diff
Mon, May 13, 5:23 PM
Unknown Object (File)
Mon, May 13, 2:57 PM
Unknown Object (File)
Thu, May 9, 5:22 PM
Unknown Object (File)
Thu, May 9, 5:22 PM
Unknown Object (File)
Thu, May 9, 5:22 PM
Unknown Object (File)
Thu, May 2, 11:14 AM
Unknown Object (File)
Thu, May 2, 11:14 AM

Details

Summary

Depends on D5073

This addresses a similar problem to the one shown in D5072. This time it refers to the attachment holders for logs.

Test Plan

services build and all tests pass (integration, performance).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jon added inline comments.
services/commtest/tests/backup_integration_test.rs
112–130 ↗(On Diff #16377)

Feel like this could just be a simple loop with a zip. No need to construct two hashmaps with the same structure over the data you're iterating.

This revision now requires changes to proceed.Sep 14 2022, 11:03 AM
karol added inline comments.
services/commtest/tests/backup_integration_test.rs
112–130 ↗(On Diff #16377)

This doesn't work, because zip doesn't overcome the order problem that I pointed out in the description of D5072 (which is referred to in the description of this very diff):

Since some logs are pulled later and some sooner, we have to identify them by IDs instead of the order in which they come from the server. For example, if you have the logs of size 3MB, 3MB, 5KB, or 3MB, first you'll probably receive this of 5KB and only then the rest.

This is why I used maps in the first place.

To demonstrate what's the issue with your code, I created a minimal code so you can see:

use std::collections::HashMap;

struct S {
  id: i32,
  value: i32,
}

fn main() {
  let v1 = vec![
    S { id: 0, value: 0 },
    S { id: 1, value: 1 },
    S { id: 2, value: 2 },
    S { id: 3, value: 3 },
  ];
  let v2 = vec![
    S { id: 3, value: 3 },
    S { id: 0, value: 0 },
    S { id: 2, value: 2 },
    S { id: 1, value: 1 },
  ];

  // this works
  {
    let mut m1 = HashMap::new();
    let mut m2 = HashMap::new();

    assert_eq!(v1.len(), v2.len());

    for i in 0..v1.len() {
      m1.insert(v1[i].id, v1[i].value);
      m2.insert(v2[i].id, v2[i].value);
    }

    for (id1, val1) in m1 {
      let val2 = m2.get(&id1).unwrap().clone();
      assert_eq!(val1, val2);
    }
  }

  // this doesn't work
  {
    for (item1, item2) in v1.iter().zip(v2) {
      assert_eq!(item1.value, item2.value);
    }
  }
}
tomek added inline comments.
services/commtest/tests/backup_integration_test.rs
112–130 ↗(On Diff #16377)

Does Rust have a group by? It could be the simplest approach.

karol edited the summary of this revision. (Show Details)

format

This revision is now accepted and ready to land.Sep 15 2022, 4:22 PM
services/commtest/tests/backup_integration_test.rs
112–130 ↗(On Diff #16377)

Just a reminder about this question

services/commtest/tests/backup_integration_test.rs
112–130 ↗(On Diff #16377)

There's something, but I don't really think this is a top priority. For me, the code is fairly clean right now, it doesn't have to be the most efficient on the planet, since there are just tests.

The goal is to ship in this stack ASAP, this code can always be improved. If you feel like this is critical nevertheless, feel free to create a task for this.

This revision was landed with ongoing or failed builds.Sep 19 2022, 1:12 AM
This revision was automatically updated to reflect the committed changes.