Integration tests should be run locally before committing code changes

changed milestone to %Tor: 0.3.1.x-final

added component::core tor/tor doc milestone::Tor: 0.3.1.x-final owner::chelseakomlo priority::medium resolution::fixed review-group-16 reviewer::teor severity::normal status::closed test triage-out-030-201612 type::enhancement labels

Trac:
Component: - Select a component to Core Tor/Tor
Keywords: N/A deleted, doc, test added
Milestone: N/A to Tor: 0.3.0.x-final

We should also make the docs as well. But because they're slow, many of us configure without asciidoc.

Replying to chelseakomlo:

Two changes could help with this:

Adding this information to documentation for contributors There is already documentation in CodingStandards.md.

Adding a make task that runs all necessary tasks before contributing new code make check is the default target for running tests so I'd move every test under this target. This will also make the jenkins builds run these tests automatically, making catching issues easier. I would even go further and encourage people to run make distcheck which tests whether the distribution works (see https://www.gnu.org/software/automake/manual/automake.html#Checking-the-Distribution).

Lastly, integrating make check-spaces into the distribution process has also been discussed in #5500 (moved).

If someone hasn't set up chutney and runs make test-network-all, the output is:

$CHUTNEY_PATH was not set.

To run these tests, git clone https://git.torproject.org/chutney.git ; export CHUTNEY_PATH=pwd/chutney

I personally think this helpful, and there are further instructions in the chutney repo. This just means that someone who wants to contribute code changes will need to set up both projects independently.

It may be worth noting that running make test-network-all requires a later version of automake. The minimum required version to build tor and run its tests is 1.11 but test-network-all needs 1.13 because it needs test-driver.

Trac:
Username: rubiate

Ok, I see. It looks like we have make test-full and make test-full-online which runs all tests. This is in WritingTests.md, but maybe we can make this info consistent in CodingStandards.md as well.

Here is latest progress:

Changes to CodingStandards.md to have similar recommendations to WritingTests.md.

We have make tests-full which runs the entire test suite. All I did was make this more consistent.

see github.com/chelseakomlo/tor_patches.git, branch testing

We talked about having one make task to cover all actions that are needed before pushing code. Is anyone in favor of this? For example, we could have a task make full, which would run check-spaces and test-full.

Running integration tests in Jenkins (using docker)

See git@github.com:chelseakomlo/tor-integration-ci.git, master branch

This just needs to be set up in Jenkins. We can also test using different OS/versions as we do with unit tests.

The blocker to this being more useful is that chutney tests are non-deterministic. For the short term, this could be a non-blocking Jenkins task.

(I can open a different issue for this if we want to separate setting up integration tests in CI)

Running integration tests locally

I did a proof of concept for this using docker (it was a minimal change from 2 above). The nice thing about this is managing setup/teardown. The downside is having a dependency on docker.

See here: git@github.com:chelseakomlo/tor-integration-local.git, master branch

This is just a proof of concept, we can keep running tests as we do now. We would need to clean up processes that are left over after tests have been run (see #20409 (moved)).

What is left to do? 1. Make chutney tests more deterministic.

First, making test as determinisitc as possible. I know we should look for race conditions, but any other examples of ways to fix flakiness are welcome.

It would also be great to have a deeper conversation about what an acceptable fail rate could be (best 2 out of 3, for example), as my understanding is that these tests can never be 100% deterministic.

2. Allow integration tests to be run offline. Tickets have already been opened for these:

https://trac.torproject.org/projects/tor/ticket/19573 https://trac.torproject.org/projects/tor/ticket/16971

Let me know any thoughts/ideas on these, particularly on making tests more deterministic. I think next steps can be 1) creating a Jenkins task for running integration tests and 2) making chutney tests more deterministic.

Trac:
Reviewer: N/A to teor

Replying to chelseakomlo:

Reviewer set to teor

I'm not sure I'll have time to review this any time soon. I may not be the best person for it, because I am not familiar with jenkins or docker.

As proposed in comment:3 make check should be the run all tests target because it is the standard Makefile target for running test suites.

IMO the other test targets should be able to gracefully handle situations where Stem, Chutney or Perl (in the case of check-spaces and check-docs) are not available. Currently none of these targets do this and just fail by either exiting with exit code 1 or simply running non-existing commands (which leads to the same result).

Replying to teor:

Replying to chelseakomlo:

Reviewer set to teor

I'm not sure I'll have time to review this any time soon. I may not be the best person for it, because I am not familiar with jenkins or docker.

Sure, that's fine. Who would you suggest to review this?

Replying to chelseakomlo:

Replying to teor:

Replying to chelseakomlo:

Reviewer set to teor

I'm not sure I'll have time to review this any time soon. I may not be the best person for it, because I am not familiar with jenkins or docker.

Sure, that's fine. Who would you suggest to review this?

I suggest you split this ticket into a subticket for each set of changes.

Your changes to the tor documentation could be reviewed by any of the current developers, or just given straight to nickm.
Your changes to jenkins and docker are best reviewed by people familiar with them - I'm not sure who that is, but weasel does admin our jenkins instances.

Replying to cypherpunks:

As proposed in comment:3 make check should be the run all tests target because it is the standard Makefile target for running test suites.

IMO the other test targets should be able to gracefully handle situations where Stem, Chutney or Perl (in the case of check-spaces and check-docs) are not available. Currently none of these targets do this and just fail by either exiting with exit code 1 or simply running non-existing commands (which leads to the same result).

You could also open another ticket to address this feedback.

Great, thanks! Will do.

Replying to cypherpunks:

Replying to chelseakomlo:

Adding a make task that runs all necessary tasks before contributing new code make check is the default target for running tests so I'd move every test under this target. This will also make the jenkins builds run these tests automatically, making catching issues easier.

I think the issue with Jenkins running make check for the entire test suite is that chutney tests are currently non-deterministic. Until we feel confident in the output from chutney tests, we should keep running these as a seperate task so that we have confidence when the build passes or fails.

Agreed that this should be the ultimate goal, but eliminating flakiness in chutney tests is currently a blocker.

Replying to cypherpunks:

IMO the other test targets should be able to gracefully handle situations where Stem, Chutney or Perl (in the case of check-spaces and check-docs) are not available. Currently none of these targets do this and just fail by either exiting with exit code 1 or simply running non-existing commands (which leads to the same result).

Thanks, good point. See #20653 (moved)

Trac:
Owner: N/A to chelseakomlo
Status: new to assigned

Triaged out on December 2016 from 030 to 031.

Trac:
Milestone: Tor: 0.3.0.x-final to Tor: 0.3.1.x-final
Keywords: N/A deleted, triage-out-030-201612 added

I think this ticket can be closed with only documentation changes. Other larger todos, such as fixing Chutney flakiness and incorporating Chutney into the Jenkins pipeline, are captured in tickets mentioned above.

Replying to teor:

I suggest you split this ticket into a subticket for each set of changes.

Your changes to the tor documentation could be reviewed by any of the current developers, or just given straight to nickm.

See documentation changes at git@github.com:chelseakomlo/tor_patches.git, branch documentation_integ_tests. This adds the recommendation to run integration tests before submitting code patches. It also recommends running make distcheck, per cypherpunks feedback above.

Your changes to jenkins and docker are best reviewed by people familiar with them - I'm not sure who that is, but weasel does admin our jenkins instances.

Great, I opened tickets for these & weasel reviewed. Docker doesn't make sense for the Jenkins pipeline, but the PoC is translatable to the work required to set up a task to run chutney on every build.

Trac:
Status: assigned to needs_review
Description: Currently, it is easy to submit a patch for code changes without first running integration tests locally.

Two changes could help with this:

Adding this information to documentation for contributors
Adding a make task that runs all necessary tasks before contributing new code, which are:

make check-spaces make check . make test-network-all

We should investigate how to handle:

Someone not having have chutney
Someone not having the necessary network connection for integration tests to succeed
If chutney tests fail because of flakiness (race conditions for example) rather than legitimate failures.

to

Currently, it is easy to submit a patch for code changes without first running integration tests locally.

Two changes could help with this:

Adding this information to documentation for contributors
Adding a make task that runs all necessary tasks before contributing new code, which are:

make check-spaces make check make test-network-all

We should investigate how to handle:

Someone not having have chutney
Someone not having the necessary network connection for integration tests to succeed
If chutney tests fail because of flakiness (race conditions for example) rather than legitimate failures.

With #5500 (moved) being implemented, make check now runs the check-spaces target. Because make distcheck also runs make check when it tests the distribution, asking developers to call make check-spaces separately is redundant. Maybe you can remove the lines mentioning make check-spaces?

Also IMHO the commit message of ff085157a5b8ecff00604e3a78295d271cda0c51 is too long.

Thanks for the feedback- see git@github.com:chelseakomlo/tor_patches.git, branch documentation_integ_tests with these changes.

Replying to cypherpunks:

With #5500 (moved) being implemented, make check now runs the check-spaces target. Because make distcheck also runs make check when it tests the distribution, asking developers to call make check-spaces separately is redundant. Maybe you can remove the lines mentioning make check-spaces?

Sure, good point. See dfde58db6b4ca0b406b872d7b049efbe07d7a0fe

Also IMHO the commit message of ff085157a5b8ecff00604e3a78295d271cda0c51 is too long.

Fair. See d95678ca8f97ac6e648598af01b4394840999e72

Replying to chelseakomlo:

Thanks for the feedback- see git@github.com:chelseakomlo/tor_patches.git, branch documentation_integ_tests with these changes.

The changes look good. I'll leave it to teor for the final review.

In the future create a new branch when you change the history of commits that are already made public (aka pushed). See the warning in the Github article about changing messages of pushed commits. The solution is to use git checkout -b new-branch-name to create and checkout a new branch before rewriting history.

Trac:
Keywords: N/A deleted, review-group-16 added

Documentation-integ-checks is fine with me. Merging. Thanks!

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

closed

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

Integration tests should be run locally before committing code changes

Child items 0

Activity