Skip to content

Commit

Permalink
core: fix linked list tests reusing Registrations (#1016)
Browse files Browse the repository at this point in the history
## Motivation

Currently, the tests for the linked list implementation in
`tracing_core::callsite` declare two static `Registration`s in the test
module, which are used in multiple tests. Since these tests are all run
in one binary, in separate threads, these statics are shared across all
tests. This means that --- depending on the order in which tests are run
--- these `Registration`s may already have values for their `next`
pointers. In particular, there's a potential issue where the `for_each`
in the test `linked_list_push` can loop repeatedly, if one of the
`Registration`s already had a next pointer from a previous test. See:
#1008 (comment)

## Solution

This branch declares separate `Registration` statics for each test.
These are not shared between multiple test threads.

We still reuse the same callsite statics, since their state does not
change during the tests --- only the `Registration`s change. I also
refactored the test code a little bit to use only a single type
implementing `Callsite`, rather than two separate types, since the
callsite impl is not actually used and this makes the code somewhat more
concise.

Finally, I added a new test for pushing more than two callsites.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
  • Loading branch information
hawkw authored Oct 5, 2020
1 parent 5fb114f commit 4707e6f
Showing 1 changed file with 67 additions and 16 deletions.
83 changes: 67 additions & 16 deletions tracing-core/src/callsite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,23 +276,11 @@ impl LinkedList {
mod tests {
use super::*;

#[derive(Eq, PartialEq)]
struct Cs1;
static CS1: Cs1 = Cs1;
static REG1: Registration = Registration::new(&CS1);
struct TestCallsite;
static CS1: TestCallsite = TestCallsite;
static CS2: TestCallsite = TestCallsite;

impl Callsite for Cs1 {
fn set_interest(&self, _interest: Interest) {}
fn metadata(&self) -> &Metadata<'_> {
unimplemented!("not needed for this test")
}
}

struct Cs2;
static CS2: Cs2 = Cs2;
static REG2: Registration = Registration::new(&CS2);

impl Callsite for Cs2 {
impl Callsite for TestCallsite {
fn set_interest(&self, _interest: Interest) {}
fn metadata(&self) -> &Metadata<'_> {
unimplemented!("not needed for this test")
Expand All @@ -301,6 +289,9 @@ mod tests {

#[test]
fn linked_list_push() {
static REG1: Registration = Registration::new(&CS1);
static REG2: Registration = Registration::new(&CS2);

let linked_list = LinkedList::new();

linked_list.push(&REG1);
Expand All @@ -325,9 +316,69 @@ mod tests {
});
}

#[test]
fn linked_list_push_several() {
static REG1: Registration = Registration::new(&CS1);
static REG2: Registration = Registration::new(&CS2);
static REG3: Registration = Registration::new(&CS1);
static REG4: Registration = Registration::new(&CS2);

let linked_list = LinkedList::new();

fn expect<'a>(
callsites: &'a mut impl Iterator<Item = &'static Registration>,
) -> impl FnMut(&'static Registration) + 'a {
move |reg: &'static Registration| {
let ptr = callsites
.next()
.expect("list contained more than the expected number of registrations!");

assert!(
ptr::eq(reg, ptr),
"Registration pointers need to match ({:?} != {:?})",
reg,
ptr
);
}
}

linked_list.push(&REG1);
linked_list.push(&REG2);
let regs = [&REG2, &REG1];
let mut callsites = regs.iter().copied();
linked_list.for_each(expect(&mut callsites));
assert!(
callsites.next().is_none(),
"some registrations were expected but not present: {:?}",
callsites
);

linked_list.push(&REG3);
let regs = [&REG3, &REG2, &REG1];
let mut callsites = regs.iter().copied();
linked_list.for_each(expect(&mut callsites));
assert!(
callsites.next().is_none(),
"some registrations were expected but not present: {:?}",
callsites
);

linked_list.push(&REG4);
let regs = [&REG4, &REG3, &REG2, &REG1];
let mut callsites = regs.iter().copied();
linked_list.for_each(expect(&mut callsites));
assert!(
callsites.next().is_none(),
"some registrations were expected but not present: {:?}",
callsites
);
}

#[test]
#[should_panic]
fn linked_list_repeated() {
static REG1: Registration = Registration::new(&CS1);

let linked_list = LinkedList::new();

linked_list.push(&REG1);
Expand Down

0 comments on commit 4707e6f

Please sign in to comment.