Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chutney ci 035 #736

Open
wants to merge 5 commits into
base: maint-0.3.5
Choose a base branch
from

Conversation

nmathewson
Copy link
Contributor

No description provided.

This make target runs a script that tries test-network.sh a few
times, giving up if we haven't had a success after a few tries.

We'll use it to make a forgiving travis target.
Previously we had "make check" launched whenever DISTCHECK was
false.  Now we'd like to turn it off in a few other circumstances,
like running chutney.  Maybe stem too?
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3996

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 12 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 60.302%

Files with Coverage Reduction New Missed Lines %
src/feature/dirauth/shared_random.c 3 85.64%
src/feature/dirauth/dirvote.c 9 64.79%
Totals Coverage Status
Change from base Build 3977: -0.02%
Covered Lines: 42854
Relevant Lines: 71066

💛 - Coveralls

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, but I think test-network-forgiving.sh belongs in chutney, not tor.

while [ "${n_tries}" -lt "${TEST_NET_RETRIES}" ]; do
n_tries=$((n_tries + 1))
# shellcheck disable=SC2086
if "${top_srcdir:-.}/src/test/test-network.sh" ${TEST_NETWORK_FLAGS}; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the script directory instead of the current working directory?

Suggested change
if "${top_srcdir:-.}/src/test/test-network.sh" ${TEST_NETWORK_FLAGS}; then
if "${top_srcdir:-`dirname -- "$0"`}/src/test/test-network.sh" ${TEST_NETWORK_FLAGS}; then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. The script directory would mean that the default was something like scripts/test/src/test/test-network.sh, which won't work. Maybe the default should be the script directory, followed by /../.. ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, we need to go back those two steps, then forward.

@@ -0,0 +1,24 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put test-network-forgiving.sh in chutney/tools rather than tor/src/test?
It would be a useful script to have in chutney, and chutney's CI may need it.
(We moved almost all of our chutney-specific scripts to chutney many releases ago. tor/src/test/test-network.sh is a stub that finds chutney/tools/test-network.sh. I want to avoid any more chutney-specific scripts in Tor.)

But if it's in chutney, how can we reliably find it from the Makefile, without duplicating the logic in tor/src/test/test-network.sh?

Maybe we could make the code in test-network-forgiving.sh part of chutney/tools/test-network.sh?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have this become part of test-network.sh eventually, and I tried to do that at first, but I think for now this is a good compromise. The issue I had was that the maint-0.2.9 version of test-network.sh and the master version of test-network.sh are pretty different, and I want to actually run the same version of test-network.sh that the associated Tor has.

Once maint-0.2.9 is EOL, I would be much less confused about folding this into the test-network.sh script that chutney has.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have this become part of test-network.sh eventually, and I tried to do that at first, but I think for now this is a good compromise. The issue I had was that the maint-0.2.9 version of test-network.sh and the master version of test-network.sh are pretty different, and I want to actually run the same version of test-network.sh that the associated Tor has.

But tor's test-network.sh calls chutney's test-network.sh?

(It certainly does on 0.3.4. And it does on my macOS machine in 0.2.9. But it seems to be broken in our 0.2.9 CI on Linux, which probably means we need to backport some fixes to 0.2.9's test-network.sh. I'll track them down.)

So I think we should modify chutney's test-network.sh in chutney master, and leave tor's test-network.sh alone.

Here's how it's supposed to work:

Since 0.2.9.1-alpha, tor's test-network.sh automatically calls chutney's test-network.sh, using the same arguments it was passed. So any new features in chutney's test-network.sh can be used by calling tor's test-network.sh with the correct arguments.

And since 0.3.1.1-alpha, there's no useful code in tor's test-network.sh: all it does is search for chutney's test-network.sh and exec it.

Once maint-0.2.9 is EOL, I would be much less confused about folding this into the test-network.sh script that chutney has.

In 0.2.9, we decided not to add any more chutney-related features to tor. I'd like to keep that policy, because moving these kinds of scripts is a pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants