Opened 3 years ago

Closed 3 years ago

#20458 closed enhancement (fixed)

Integration tests should be run locally before committing code changes

Reported by: chelseakomlo Owned by: chelseakomlo
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: test, doc, triage-out-030-201612, review-group-16
Cc: Actual Points:
Parent ID: Points:
Reviewer: teor Sponsor:

Description (last modified by chelseakomlo)

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

Two changes could help with this:

  1. Adding this information to documentation for contributors
  2. 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.

Child Tickets

Change History (25)

comment:1 Changed 3 years ago by dgoulet

Component: - Select a componentCore Tor/Tor
Keywords: test doc added
Milestone: Tor: 0.3.0.x-final

comment:2 Changed 3 years ago by teor

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

comment:3 in reply to:  description ; Changed 3 years ago by cypherpunks

Replying to chelseakomlo:

Two changes could help with this:

  1. Adding this information to documentation for contributors

There is already documentation in CodingStandards.md.

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

comment:4 Changed 3 years ago by chelseakomlo

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.

comment:5 Changed 3 years ago by rubiate

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.

comment:6 Changed 3 years ago by chelseakomlo

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.

comment:7 Changed 3 years ago by chelseakomlo

Here is latest progress:

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

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

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

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.

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



Last edited 3 years ago by chelseakomlo (previous) (diff)

comment:8 Changed 3 years ago by chelseakomlo

Reviewer: teor

comment:9 in reply to:  8 ; Changed 3 years ago by 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.

comment:10 Changed 3 years ago by 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).

comment:11 in reply to:  9 ; Changed 3 years ago by 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?

comment:12 in reply to:  11 Changed 3 years ago by teor

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.

comment:13 in reply to:  10 Changed 3 years ago by teor

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.

comment:14 Changed 3 years ago by chelseakomlo

Great, thanks! Will do.

comment:15 in reply to:  3 Changed 3 years ago by chelseakomlo

Replying to cypherpunks:

Replying to chelseakomlo:

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

comment:16 in reply to:  10 Changed 3 years ago by chelseakomlo

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

comment:17 Changed 3 years ago by chelseakomlo

Owner: set to chelseakomlo
Status: newassigned

comment:18 Changed 3 years ago by dgoulet

Keywords: triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

Triaged out on December 2016 from 030 to 031.

comment:19 Changed 3 years ago by chelseakomlo

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.

comment:20 Changed 3 years ago by chelseakomlo

Description: modified (diff)
Status: assignedneeds_review

comment:21 Changed 3 years ago by cypherpunks

With #5500 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.

comment:22 in reply to:  21 ; Changed 3 years ago by chelseakomlo

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

Replying to cypherpunks:

With #5500 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

comment:23 in reply to:  22 Changed 3 years ago by cypherpunks

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.

comment:24 Changed 3 years ago by nickm

Keywords: review-group-16 added

comment:25 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

Note: See TracTickets for help on using tickets.