Opened 2 years ago

Closed 2 years ago

#22762 closed task (implemented)

Revise coding standards expectation for tests to be run before review

Reported by: chelseakomlo Owned by: chelseakomlo
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: documentation
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

There are several make targets with varying combinations of tests that are run.

Currently, we expect contributors to run the full test suite, including integration tests. This is a lot to ask, especially for new contributors.

Instead, let's require make check, but advise make test-full on feature changes and larger commits.

Child Tickets

Change History (14)

comment:1 Changed 2 years ago by chelseakomlo

I have a very small patch here:

git@github.com:chelseakomlo/tor_patches.git, branch coding-standards-22762

Any other suggestions for how to improve test requirements before code submission/review, happy to include.

comment:2 Changed 2 years ago by chelseakomlo

Status: newneeds_review

comment:3 Changed 2 years ago by catalyst

Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Status: needs_reviewmerge_ready

Thanks! The patch looks good to me. It might need a changes file? (maybe under "Documentation" or "Testing"?)

comment:4 Changed 2 years ago by cypherpunks

Running make check is redundant when you ask contributors to also run make distcheck. The later runs make check as one of its steps (see https://www.gnu.org/software/automake/manual/automake.html#Checking-the-Distribution).

comment:5 in reply to:  4 Changed 2 years ago by chelseakomlo

Replying to cypherpunks:

Running make check is redundant when you ask contributors to also run make distcheck. The later runs make check as one of its steps (see https://www.gnu.org/software/automake/manual/automake.html#Checking-the-Distribution).

Cool, that is a great point, thanks for the feedback.

catalyst, what do you think about just requiring make distcheck, with a brief explanation that this task runs both the test suite and checks the distribution?

comment:6 Changed 2 years ago by catalyst

I think make distcheck is too cumbersome for ordinary contributors to run on every patch they submit. (At the very least, it takes significantly longer to run.) It builds a distribution tarball, does a build from that tarball (which easily takes much longer than an incremental build), runs make check, tests the installation rules/scripts, etc. I think it also won't use any custom ./configure options such as --enable-fragile-hardening that might help a developer find bugs in new code. I'm not sure whether make distcheck has more dependencies than make check, but if it does, that might be another reason to not require it from all contributors.

For comparison, the Jenkins builds do only make check not make distcheck. I think it only makes sense to do make distcheck when there are changes to build system components.

I think make check is a reasonable tradeoff between thoroughness and something that runs quickly enough that contributors would be willing to run it.

Maybe we could additionally recommend running make distcheck if you've touched build system stuff (like Makefiles or autoconf)?

comment:7 Changed 2 years ago by nickm

That sounds good to me

comment:8 Changed 2 years ago by cypherpunks

Also sounds good to me.

comment:9 Changed 2 years ago by chelseakomlo

Sure, that sounds good.

I can make that change in a separate commit, unless you think it is best to open another ticket for this.

comment:10 Changed 2 years ago by nickm

A separate commit is fine; thanks!

comment:11 Changed 2 years ago by nickm

Status: merge_readyassigned

Merged the main commit to master; putting this back in assigned.

comment:12 Changed 2 years ago by chelseakomlo

I pushed the commit for when to run make distcheck to:

git@github.com:chelseakomlo/tor_patches.git, branch coding-standards-22762, commit 934f85f87aa17bf6e1ffad37df8d8d4fb7bf404e

Let me know if there is any changes or additions I can make.

comment:13 Changed 2 years ago by chelseakomlo

Status: assignedneeds_review

comment:14 Changed 2 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged!

Note: See TracTickets for help on using tickets.