Opened 4 weeks ago

Closed 7 days ago

#31477 closed defect (fixed)

Practracker integration tests for headers and includes

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: practracker, tech-debt, refactoring, easy, 041-deferred-20190530, network-team-roadmap-july, dgoulet-merge
Cc: nickm, teor, asn Actual Points:
Parent ID: #29746 Points:
Reviewer: catalyst Sponsor: Sponsor31-must

Description

We added new practracker features in #31175 and #31176, but those were written before practracker had integration tests (#31263). We should add them to the practracker tests.

Child Tickets

Change History (9)

comment:1 Changed 3 weeks ago by nickm

Status: assignedneeds_review

Done in ticket31477 with PR in https://github.com/torproject/tor/pull/1264 .

I found a bug in our header-file dependency-violation checking, and fixed it. Because of that, I had to add some new exceptions.

comment:2 Changed 3 weeks ago by dgoulet

Reviewer: catalyst

comment:3 in reply to:  1 Changed 11 days ago by catalyst

Status: needs_reviewneeds_revision

Replying to nickm:

Done in ticket31477 with PR in https://github.com/torproject/tor/pull/1264 .

I found a bug in our header-file dependency-violation checking, and fixed it. Because of that, I had to add some new exceptions.

Thanks! Could you please add a brief description of that bug both to this ticket and to the appropriate commit messages? Please force-push the pull request branch once you've done this.

I'm also undecided on whether this needs a changes file. What do you think?

comment:4 Changed 11 days ago by nickm

Status: needs_revisionneeds_review

Thanks! Could you please add a brief description of that bug both to this ticket and to the appropriate commit messages? Please force-push the pull request branch once you've done this.

Good idea. I've done that and force-pushed.

The bug is this: I thought that saying that DependencyViolation applied to * would make it apply to all files -- but practracker doesn't support that. I had to say instead that it applies to *.h and *.c.

I force-pushed a new branch to fix the commit message. I didn't change any code when I did so.

I'm also undecided on whether this needs a changes file. What do you think?

I'm not sure whether your "this" means this branch, or this bug. I don't think that the bug requires a changes file, since the bug didn't exist in 0.4.1. But I think that this branch needs a changes file, since it is a new test that wasn't there before. Sorry I hadn't added it! I've just put it in the branch.

comment:5 in reply to:  4 ; Changed 11 days ago by catalyst

Replying to nickm:

The bug is this: I thought that saying that DependencyViolation applied to * would make it apply to all files -- but practracker doesn't support that. I had to say instead that it applies to *.h and *.c.

I still didn't understand this description, so I looked into practracker.py and problem.py. I'm guessing that ProblemFilter needs to have its filename patterns be unambiguously denoting either .c or .h files. It looks like Item.get_file_type() determines this based on the suffix (.c or .h) of the file or pattern, defaulting to *.c. So maybe a pattern of * would match *.c files but not *.h files? Does this sound like an accurate description of the bug?

I force-pushed a new branch to fix the commit message. I didn't change any code when I did so.

I'm also undecided on whether this needs a changes file. What do you think?

I'm not sure whether your "this" means this branch, or this bug. I don't think that the bug requires a changes file, since the bug didn't exist in 0.4.1. But I think that this branch needs a changes file, since it is a new test that wasn't there before. Sorry I hadn't added it! I've just put it in the branch.

Thanks!

comment:6 in reply to:  5 ; Changed 7 days ago by nickm

Replying to catalyst:

Replying to nickm:

The bug is this: I thought that saying that DependencyViolation applied to * would make it apply to all files -- but practracker doesn't support that. I had to say instead that it applies to *.h and *.c.

I still didn't understand this description, so I looked into practracker.py and problem.py. I'm guessing that ProblemFilter needs to have its filename patterns be unambiguously denoting either .c or .h files. It looks like Item.get_file_type() determines this based on the suffix (.c or .h) of the file or pattern, defaulting to *.c. So maybe a pattern of * would match *.c files but not *.h files? Does this sound like an accurate description of the bug?

That's right -- the Item.get_file_type() function converts * to *.c, so that it will match .c files, but not .h files.

comment:7 Changed 7 days ago by catalyst

Summary: Practracker integration tsts for headers and includesPractracker integration tests for headers and includes

fix typo in summary

comment:8 in reply to:  6 Changed 7 days ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

Replying to catalyst:

Replying to nickm:

The bug is this: I thought that saying that DependencyViolation applied to * would make it apply to all files -- but practracker doesn't support that. I had to say instead that it applies to *.h and *.c.

I still didn't understand this description, so I looked into practracker.py and problem.py. I'm guessing that ProblemFilter needs to have its filename patterns be unambiguously denoting either .c or .h files. It looks like Item.get_file_type() determines this based on the suffix (.c or .h) of the file or pattern, defaulting to *.c. So maybe a pattern of * would match *.c files but not *.h files? Does this sound like an accurate description of the bug?

That's right -- the Item.get_file_type() function converts * to *.c, so that it will match .c files, but not .h files.

Thanks. Now that explanation is in the ticket so it's a little easier to discover.

comment:9 Changed 7 days ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.