Skip to content

Branch coverage 100% - #3594

Open
amusingimpala75 wants to merge 33 commits into
commaai:masterfrom
amusingimpala75:branch-coverage
Open

Branch coverage 100%#3594
amusingimpala75 wants to merge 33 commits into
commaai:masterfrom
amusingimpala75:branch-coverage

Conversation

@amusingimpala75

@amusingimpala75 amusingimpala75 commented Jul 27, 2026

Copy link
Copy Markdown

I noticed the branch coverage bounty about two weeks ago, and not seeing any work on it listed in a few months I figured I'd give it a go.

Completes #2557 (and I can actually say that if you check out the branch and run the tests you will in fact achieve 100% on the report, unlike some of the AI PRs). I developed this on an M1 MacBook Air, I don't know if that might cause any regressions in coverage.

Notes:

  • I disabled the branch checks for the SAFETY_(MIN|MAX|CLAMP|ABS) as I felt that those are sufficiently obvious that we don't need to check for them and it would be a ton of needless checks that I didn't want to bother with.
  • I added a couple extra setters/getters for properties, although I wasn't sure if that's okay so they're named with test at the beginning. I'll remove the prefixes on the names if it is acceptable to keep them.
  • I can rebase on top of master but I wanted to keep some amount of history of working on it chronologically rather than having all the times squashed to the same minute
  • I would not be surprised if some of the changes I made are asked to be refined. For example, if there was a bus and address check but the address was only registered for a single bus I removed the bus check as it was unnecessary, but I could see that being not desired if there is a decent chance of modification later. I dunno
  • AI assistance was used in the exploration of the codebase, but was limited for the code writing; I think I used it in one case which I noted in that corresponding commit.
  • I recognize the number of commits may not be ideal; again, I found it helpful for organising while coding but can squash it before merging.
  • I wrote the Tesla modifications to use a switch because I felt that was the best way to do it (chains of comparing one variable to various values). If that is considered acceptable I would like to suggest re-writing some sections to use switch statements.

I am of course ready to go through the back-and-forth of code review, so if there are any changes that should be made for the completion of the PR/bounty please let me know!

I'm new to this so this might be wrong, but for
Chrysler address 514 for is only on bus 514 (0x202)

(and then I squash these rest)

safety/gm: remove redundant bus check
safety/ford: remove redundant bus check
safety/mazda: remove redundant bus checks
safety/subaru: remove redundant bus checks
safety/toyota: remove redundant bus check
safety/volkwagen: remove redundant bus checks
I don't actually know if this was required since that was unrelated to branching, but oh well
I don't know if this is quite okay, but those functions are fundamental and weird to test
from the python, so I hope its okay
I'm not sold on how I did this, but it does hit 100% for the file
It is acceptable to remove the gm_pcm_cruise branch in
gm.h because (at least currently) gm_pcm_cruise is true
iff gm_hw == GM_CAM iff 0x1E1 has a defined tx_check.
Removing the longitudinal check /should/ be fine
since the two callers already ensure that it isn't
longitudinal
I used Terra for the safety_tick_rx_check ones, but it
seems reasonable and matches the test cases
The names were swapped for the raw vs val which was
a bit of a pain, so I fixed it.
@github-actions github-actions Bot added the car safety vehicle-specific safety code label Jul 27, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing to opendbc! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • include a route or your device' dongle ID if relevant

@amusingimpala75
amusingimpala75 marked this pull request as draft July 27, 2026 20:13
@amusingimpala75

Copy link
Copy Markdown
Author

Huh, it looks like my usage of llvm-cov instead of gc-cov (or whatever its called) is causing discrepancies, even though thats the current default here

@amusingimpala75

Copy link
Copy Markdown
Author

Nope, the issue is actually the machine architecture, I guess arm simply has better machine code

@amusingimpala75

amusingimpala75 commented Jul 28, 2026

Copy link
Copy Markdown
Author

I got to use WSL to get the rest of it over the finish line lol. I'm gonna mark this as ready for review because although the last test cancelled it was just because it timed out for the car diff job to finish before it would comment it here (and the dependency action has succeeded now but I can't figure out how to have it re-run the cancelled check like I can when a commit check fails).

@amusingimpala75
amusingimpala75 marked this pull request as ready for review July 28, 2026 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

car safety vehicle-specific safety code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant