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

Add address field to SourceLocation struct #347

Closed
wants to merge 1 commit into from
Closed

Add address field to SourceLocation struct #347

wants to merge 1 commit into from

Conversation

shnewto
Copy link
Contributor

@shnewto shnewto commented Feb 16, 2020

👋 Hello! I took a stab at a fix for an issue I've seen in my own crates, thanks for taking a look 😄

Because address is used to determine if a line has been covered, this
commit's changes add address to the key used for maps of TracerData.

The gimli object used to describe line and path can be associated with multiple addresses
which resulted in scenarios where lines that were covered were missed
in the reports (because a TracerData associated with line and path, but
not the expected address, got to the map first).

In this commit, a test was added to the tests/data/matches crate to illustrate a
scenario that shows a line that was previously uncovered in reporting,
is now covered.

I was personally motivated by the match scenario the test crate changes describe, but I'm curious whether these changes result in any wins for coverage in issues like #136 or #262

Becasue address is used to determine if a line has been covered, this
commit's changes add address to the key used for maps of TracerData.

The gimli object used to describe line and path can be associated with multiple addresses
which resulted in scenarios where lines that _were_ covered were missed
in the reports (because a TracerData associated with line and path, but
not the expected address, got to the map first).

In this commit, a test was added to the `tests/data/matches` crate to illustrate a
scenario that shows a line that was previously uncovered in reporting,
is now covered.
@shnewto
Copy link
Contributor Author

shnewto commented Feb 16, 2020

For some context on what I mean about the potential for multiple addresses associated with TracerData for a single path and line, here are the candidates for TracerData from the test file modified in this PR. Before these changes, if there were multiple possibilities for a line, only one made it in (path was omitted as it was all the same lib.rs in this case).

line: 6
TracerData { trace_type: FunctionEntry(80), address: Some(4207952), length: 1, fn_name: Some("check_libtype_match") }

line: 8
TracerData { trace_type: Unknown, address: Some(4207965), length: 1, fn_name: None }

line: 7
TracerData { trace_type: Unknown, address: Some(4207978), length: 1, fn_name: None }

line: 7
TracerData { trace_type: Unknown, address: Some(4208000), length: 1, fn_name: None }

line: 8
TracerData { trace_type: Unknown, address: Some(4207980), length: 1, fn_name: None }

line: 9
TracerData { trace_type: Unknown, address: Some(4208002), length: 1, fn_name: None }

line: 11
TracerData { trace_type: Unknown, address: Some(4208022), length: 1, fn_name: None }

line: 11
TracerData { trace_type: FunctionEntry(177), address: Some(4208032), length: 1, fn_name: Some("check_match") }

line: 13
TracerData { trace_type: FunctionEntry(177), address: Some(4208032), length: 1, fn_name: Some("check_match") }

line: 14
TracerData { trace_type: Unknown, address: Some(4208090), length: 1, fn_name: None }

line: 14
TracerData { trace_type: Unknown, address: Some(4208155), length: 1, fn_name: None }

line: 14
TracerData { trace_type: Unknown, address: Some(4208166), length: 1, fn_name: None }

line: 14
TracerData { trace_type: Unknown, address: Some(4208188), length: 1, fn_name: None }

line: 15
TracerData { trace_type: Unknown, address: Some(4208041), length: 1, fn_name: None }

line: 15
TracerData { trace_type: Unknown, address: Some(4208146), length: 1, fn_name: None }

line: 16
TracerData { trace_type: Unknown, address: Some(4208081), length: 1, fn_name: None }

line: 16
TracerData { trace_type: Unknown, address: Some(4208124), length: 1, fn_name: None }

line: 17
TracerData { trace_type: Unknown, address: Some(4208157), length: 1, fn_name: None }

line: 18
TracerData { trace_type: Unknown, address: Some(4208092), length: 1, fn_name: None }

line: 18
TracerData { trace_type: Unknown, address: Some(4208168), length: 1, fn_name: None }

line: 19
TracerData { trace_type: Unknown, address: Some(4208190), length: 1, fn_name: None }

line: 21
TracerData { trace_type: Unknown, address: Some(4208199), length: 1, fn_name: None }

line: 21
TracerData { trace_type: Unknown, address: Some(4208209), length: 1, fn_name: None }

line: 23
TracerData { trace_type: FunctionEntry(115), address: Some(4208224), length: 1, fn_name: Some("destructuring_match") }

line: 24
TracerData { trace_type: Unknown, address: Some(4208236), length: 1, fn_name: None }

line: 24
TracerData { trace_type: Unknown, address: Some(4208269), length: 1, fn_name: None }

line: 24
TracerData { trace_type: Unknown, address: Some(4208279), length: 1, fn_name: None }

line: 24
TracerData { trace_type: Unknown, address: Some(4208289), length: 1, fn_name: None }

line: 25
TracerData { trace_type: Unknown, address: Some(4208252), length: 1, fn_name: None }

line: 25
TracerData { trace_type: Unknown, address: Some(4208326), length: 1, fn_name: None }

line: 26
TracerData { trace_type: Unknown, address: Some(4208261), length: 1, fn_name: None }

line: 26
TracerData { trace_type: Unknown, address: Some(4208291), length: 1, fn_name: None }

line: 27
TracerData { trace_type: Unknown, address: Some(4208271), length: 1, fn_name: None }

line: 27
TracerData { trace_type: Unknown, address: Some(4208317), length: 1, fn_name: None }

line: 28
TracerData { trace_type: Unknown, address: Some(4208281), length: 1, fn_name: None }

line: 30
TracerData { trace_type: Unknown, address: Some(4208334), length: 1, fn_name: None }

line: 30
TracerData { trace_type: Unknown, address: Some(4208339), length: 1, fn_name: None }

line: 37
TracerData { trace_type: TestEntry(17), address: Some(4208448), length: 1, fn_name: Some("{{closure}}") }

line: 37
TracerData { trace_type: Unknown, address: Some(4208453), length: 1, fn_name: None }

line: 37
TracerData { trace_type: TestEntry(183), address: Some(4207760), length: 1, fn_name: Some("it_works") }

line: 38
TracerData { trace_type: Unknown, address: Some(4207764), length: 1, fn_name: None }

line: 39
TracerData { trace_type: Unknown, address: Some(4207797), length: 1, fn_name: None }

line: 41
TracerData { trace_type: Unknown, address: Some(4207834), length: 1, fn_name: None }

line: 42
TracerData { trace_type: Unknown, address: Some(4207839), length: 1, fn_name: None }

line: 43
TracerData { trace_type: Unknown, address: Some(4207849), length: 1, fn_name: None }

line: 44
TracerData { trace_type: Unknown, address: Some(4207859), length: 1, fn_name: None }

line: 45
TracerData { trace_type: Unknown, address: Some(4207869), length: 1, fn_name: None }

line: 47
TracerData { trace_type: Unknown, address: Some(4207879), length: 1, fn_name: None }

line: 48
TracerData { trace_type: Unknown, address: Some(4207894), length: 1, fn_name: None }

line: 49
TracerData { trace_type: Unknown, address: Some(4207909), length: 1, fn_name: None }

line: 50
TracerData { trace_type: Unknown, address: Some(4207923), length: 1, fn_name: None }

line: 51
TracerData { trace_type: Unknown, address: Some(4207938), length: 1, fn_name: None }

line: 51
TracerData { trace_type: Unknown, address: Some(4207943), length: 1, fn_name: None }

line: 51
TracerData { trace_type: Unknown, address: Some(4208463), length: 1, fn_name: None }

line: 51
TracerData { trace_type: Unknown, address: Some(4208465), length: 1, fn_name: None }

@shnewto
Copy link
Contributor Author

shnewto commented Feb 16, 2020

Just poking around a little, after these changes it looks like the method_calls_expr_coverage tests pass.

@xd009642
Copy link
Owner

So the GitHub pr view in the app is kinda bad so I can't see what you branched off. But have you tried the latest stuff I committed in develop which also fixed the coverage for method calls and the futures test?

It sounds like we might have done the same thing albeit in different ways

@xd009642
Copy link
Owner

I'll have a proper look when I'm next at my pc 😊

@shnewto
Copy link
Contributor Author

shnewto commented Feb 16, 2020

I branched off current develop and yep the futures tests pass there already 🙃. The method_calls_expr_coverage tests are still failing on develop for me though.

@xd009642
Copy link
Owner

Ah that's because I'm a muppet who forgets to push his changes!

So I took a slightly different approach and stored the location the same way but made a list of traces with all the addresses as the value. I did that because I wrote some test to see how the TraceMap amount_coverable and amount_covered seemed thrown off by two traces with the same location in the structure so I was concerned it might cause issues in some parts of the report module. But that could have always been me not trusting myself 😅

@shnewto
Copy link
Contributor Author

shnewto commented Feb 16, 2020

Ohh haha, okay! I'm happy a fix made it in :)

@shnewto shnewto closed this Feb 16, 2020
@shnewto shnewto deleted the address-some-missing-coverage-reports branch February 16, 2020 20:47
@xd009642
Copy link
Owner

The test for matches is still useful 😄 if you want to submit that as another PR

@shnewto
Copy link
Contributor Author

shnewto commented Feb 16, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants