Opened 5 years ago

Closed 5 years ago

#13119 closed defect (implemented)

Convert test_* macros to tt_* macros

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I'm rebasing andrea's patch from #9262 and I'm updating it to use test_eq_ptr() rather than test_eq() when comparing function pointers.

But in git commit 31f6806aa I see Nick picking tt_ptr_op(v, ==, NULL); rather than test_eq_ptr(v, NULL);.

Is this a style thing? Should we try to be more uniform?

Child Tickets

Change History (8)

comment:1 Changed 5 years ago by nickm

They're equivalent; The tt_* macros are newer and preferred IMO. The test_* macros are older. I think this is someplace that we might try to get coccinelle to generate a sane patch for us: doing it by hand is nuts IMO.

comment:2 Changed 5 years ago by nickm

Status: newneeds_revision
Summary: guidance on tt_ptr_op vs test_eq_ptr()?Convert test_* macros to tt_* macros

So, tinytest provides tt_*; the test_* macros are for backward compatibility.

But now that I kinda know spatch, we can just migrate them.

See branch "ticket13119" in my public repo. Note that the big patch there was produced automatically; the scripts are included.

The only reason I can think of not to do this would be if we're concerned about pending unmerged patches that want to do heavy modifications to the legacy tests.

I'm marking this "needs-revision" since the patch didn't catch every instance. I can investigate why if people think this is worthwhile.

comment:3 Changed 5 years ago by nickm

current diagnosis: spatch doesn't like to modify functions that contain an operator like "==" used as a macro argument. So, perl, then spatch, then perl again maybe.

comment:4 Changed 5 years ago by nickm

Status: needs_revisionneeds_review

For review: ticket_13119_v3 . Do we pull the trigger on this?

comment:5 Changed 5 years ago by Sebastian

I don't see why not. What's the worst that could happen? For unittests, I mostly learn by looking at other tests around it, so having it consistent is a great thing.

comment:6 Changed 5 years ago by nickm

What's the worst that could happen?

"The only reason I can think of not to do this would be if we're concerned about pending unmerged patches that want to do heavy modifications to the legacy tests."

Also, it's conceivable that I messed something up here and now one of our tests is subtly less test-ful than it was before.

But personally, I lean "merge".

comment:7 Changed 5 years ago by nickm

I've added another coccinelle-assisted patch to the branch that removes all uses of legacy_test_helper.

comment:8 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged this. When I run into merge trouble, I hope I remember how nice it will be not to get any more tests using deprecated functions.

Note: See TracTickets for help on using tickets.