Revise coding standards expectation for tests to be run before review

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.

changed milestone to %Tor: 0.3.2.x-final

added component::core tor/tor documentation milestone::Tor: 0.3.2.x-final owner::chelseakomlo priority::medium resolution::implemented reviewer::catalyst severity::normal status::closed type::task version::tor unspecified labels

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.

Trac:
Status: new to needs_review

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

Trac:
Milestone: Tor: unspecified to Tor: 0.3.2.x-final
Status: needs_review to merge_ready

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).

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?

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)?

That sounds good to me

Also sounds good to me.

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.

A separate commit is fine; thanks!

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

Trac:
Status: merge_ready to assigned

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.

Trac:
Status: assigned to needs_review

Merged!

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed

moved to tpo/core/tor#22762 (closed)