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

Some misses in coverage #136

Open
vberger opened this Issue Jul 19, 2018 · 13 comments

Comments

Projects
None yet
9 participants
@vberger
Copy link
Contributor

vberger commented Jul 19, 2018

On my last coverage I've spotted a few oddities in the coverage:

an else alone on its line is the only uncovered line of the function

see https://codecov.io/gh/Smithay/wayland-rs/src/master/wayland-commons/src/socket.rs#L33

The content of both branches of an if is marked as covered, but the line containing only } else { is marked as uncovered.

a let v = expression split onto two lines

see https://codecov.io/gh/Smithay/wayland-rs/src/master/wayland-commons/src/socket.rs#L198

the code is basically

let var =
        something();

The line containing let var = is marked as uncovered, while the second one is marked as covered.

chained operations on iterators are not consistently

see https://codecov.io/gh/Smithay/wayland-rs/src/master/wayland-commons/src/wire.rs#L243

I have something like

let arguments = signature
    .iter()
    .map(|a| {
        /* some very big closure */
    })
    .collect();

the let ... lines and .map( are marked covered, while the iter() and collect() ones are marked as not covered.

a match arm marked uncovered while its contents are covered

see https://codecov.io/gh/Smithay/wayland-rs/src/master/wayland-commons/src/wire.rs#L259

It is something like

match val {
    Match1 => something(),
    Match2 => {
        another_thing();
    }
}

the line with another_thing() is marked covered, by the line with Match2 => { is marked as not covered.

EDIT: Checklist!

  • Inconsistent coverage with macros like assert_eq!
  • fix collect::<_>()? and other issues with ? on function calls
  • inline asm! - potentially just ignore this and ensure the block is hit.
  • unsafe blocks can still be weird, look into let x = unsafe {\n
  • match arms with constants in
  • lines in match which are just the pattern
  • multiline let statements
  • lines only containing else
  • Handle fact attributes can be attached to any expression
  • PhantomData
@xd009642

This comment has been minimized.

Copy link
Owner

xd009642 commented Jul 19, 2018

So most of these I'm already aware of via #63 the let expression one I kind of expected but hadn't seen any examples.

With the DWARF tables multiple instructions map to the same line and some of them if instrumented will be hit some won't. I know that can fix the chained operators on iterators from some experiments I've done. However, I haven't yet found any sources on what instructions to prefer or a way of determining the best instruction analytically...

I'm going to keep trying to figure it out but any help anyone can offer is appreciated

@xd009642

This comment has been minimized.

Copy link
Owner

xd009642 commented Oct 2, 2018

So hope you don't mind I've added a checklist to the top comment just so I can see progress from the main issues page 👍

@xd009642

This comment has been minimized.

Copy link
Owner

xd009642 commented Oct 15, 2018

So without any work of my own I've just tested the multiline let statements and lines containing only } else { and found they didn't show up in coverage results.

As the compiler changes I sometimes find coverage issues appear and disappear based on the outputted assembly so unless anyone can show any examples of them still existing I'll keep them checked off.

I might need to sanity check on some bigger projects, I've not managed to recreate the collect issue on any example projects. Yeah issues still occur, just gonna have to work harder to recreate them.

@TheDan64

This comment has been minimized.

Copy link

TheDan64 commented Oct 15, 2018

Are there still plans to build a regression test suite? And what are the best ways for the community to help you improve coverage?

@xd009642

This comment has been minimized.

Copy link
Owner

xd009642 commented Oct 15, 2018

Well right now I'm running tarpaulin on faktory-rs so I can go over the reports and look for any coverage issues.

At some point I would like the integration tests to be a lot better, completely overhaul the projects. It would need an amount of work into figuring out what coverage should look like. Obviously some of the like the proc-macro test and simple-project currently serve a purpose.

I think a good starting point for tests would be a project for each expression in https://docs.rs/syn/0.15.11/syn/enum.Expr.html that demonstrates every form each expression could take and what is expected from coverage. I'll try and do some tests like this some point this week to work as a template and we'll see where it goes from there. If I do I'll post any follow up on #54 so as not to distract too much from this issue (although they are obviously interlinked)

@jeremydavis519

This comment has been minimized.

Copy link
Contributor

jeremydavis519 commented Oct 26, 2018

I have some more data to add to this from my own recent test. This is using the published version of tarpaulin, so some of these may have been fixed already. I've marked the lines that tarpaulin said were covered:

impl Parse for CustomAssert {
>   fn parse(input: ParseStream) -> parse::Result<Self> {
        let expr = input.call(Expr::parse)?; // Required expression
        if input.parse::<Token![,]>().is_ok() { // Optional message
>           let message = input.call(Punctuated::parse_separated_nonempty)?;
>           Ok(CustomAssert { expr, message })
>       } else {
>           Ok(CustomAssert { expr, message: Punctuated::new() })
        }
    }
}

The code actually took the else branch, so the if branch shouldn't be shown as covered.

But the hits after this snippet in the same file were where I expected, so I don't know if the hits were actually being reported 2 lines ahead of where they should be. The other option is that the first two lines weren't registered and the tracer mistakenly grouped the else branch with something else.

Edit: Those were the results when I called launch_tarpaulin from my own code. The report from running cargo tarpaulin actually shows everything covered except this:

            let message = input.call(Punctuated::parse_separated_nonempty)?;
            Ok(CustomAssert { expr, message })
        } else {

I'm not sure why they would be different.

@dgriffen

This comment has been minimized.

Copy link

dgriffen commented Feb 7, 2019

One thing I'd like to add is that the --ignore-panics flag doesn't appear to work for match arms in tests with panics. See here: https://codecov.io/gh/dgriffen/nestor/src/master/nestor/src/handler.rs#L242

@jonhoo

This comment has been minimized.

Copy link

jonhoo commented Mar 2, 2019

@xd009642 here's another coverage test for inferno that has some anomalies that may serve as handy test-cases: https://codecov.io/gh/jonhoo/inferno/src/master/src/collapse/perf.rs

@calidion

This comment has been minimized.

Copy link

calidion commented Mar 4, 2019

panic lines are missed too.

@jonhoo

This comment has been minimized.

Copy link

jonhoo commented Mar 8, 2019

I think #185 is related to this.

@orium

This comment has been minimized.

Copy link

orium commented Mar 10, 2019

Another one: on inlined functions, the first line (i.e. the one with the fn keywork) will be reported as not covered. This was tested with both the latest release as of this time (0.7.0) and with develop 2620220.

To reproduce:

#[inline(always)]
fn foo() {
    println!()
}

#[test]
fn test_foo() {
    foo();
}

Line 2 (fn foo() {) will be reported as not covered. (The body is correctly reported as covered.)

Ref #79

@jonhoo

This comment has been minimized.

Copy link

jonhoo commented Mar 11, 2019

It looks like break is also missed.

@andybitter

This comment has been minimized.

Copy link

andybitter commented Mar 12, 2019

...aaaand another one:

miss:

pub fn receive_event(&mut self, event: Event) {
     let old_state = self.state.clone();

     match event {
         Event::NewClimateData(climate) => {
             self.state = self.state.determine_next_state_by_climate_input(climate)
         }
         Event::DeactivateAlarmRequested => {  // THIS LINE IS MISSED
             self.state = self.state.determine_next_state_for_deactivate_alarm_requested()
         }
     }

     if old_state != self.state {
         self.trigger_output_for_new_state();
     }
 }

works fine:

pub fn receive_event(&mut self, event: Event) {
        let old_state = self.state.clone();

        match event {
            Event::NewClimateData(climate) => {
                self.state = self.state.determine_next_state_by_climate_input(climate)
            }
            Event::DeactivateAlarmRequested => { self.state = self.state.determine_next_state_for_deactivate_alarm_requested() } // THIS WORKS FINE
        }

        if old_state != self.state {
            self.trigger_output_for_new_state();
        }
    }

swapping the two match arms also works fine:

pub fn receive_event(&mut self, event: Event) {
        let old_state = self.state.clone();

        match event {
            Event::DeactivateAlarmRequested => {
                self.state = self.state.determine_next_state_for_deactivate_alarm_requested()
            }
            Event::NewClimateData(climate) => {
                self.state = self.state.determine_next_state_by_climate_input(climate)
            }

        }

        if old_state != self.state {
            self.trigger_output_for_new_state();
        }
    }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.