Opened 11 months ago

Closed 9 months ago

#32705 closed defect (fixed)

test_practracker.sh python issues

Reported by: nickm Owned by: teor
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci-fixed-fail jenkins python network-team-roadmap-2020Q1 043-should
Cc: teor Actual Points: 0.5
Parent ID: Points: 0.1
Reviewer: dgoulet Sponsor:

Description

It looks like we have a failure on Jenkins: https://jenkins.torproject.org/job/tor-ci-linux-master/ARCHITECTURE=amd64,SUITE=bullseye/4402/consoleFull

It seems like when we check-practracker-unit-tests, we try to run it even when we don't have python installed. Additionally, we don't set the PYTHON environment variable when we run check-pracktracker-unit-tests, so the script can look for python with the wrong name.

Both of these problems are solved if we remove check-practracker-unit-tests, I believe, since test_practracker.sh is already listed in TESTSCRIPTS, so we don't acutally need to have it in check-local as we thought in #32609

Child Tickets

TicketStatusOwnerSummaryComponent
#31796closednickmpractracker git hook warning: Unusual pattern permitted.h in testdataCore Tor/Tor

Change History (20)

comment:1 Changed 11 months ago by nickm

Keywords: tor-ci jenkins python added

comment:2 Changed 11 months ago by teor

Keywords: tor-ci-fail added; tor-ci removed
Owner: set to teor
Status: newassigned

comment:3 Changed 10 months ago by dgoulet

So I get this when running scripts/maint/practracker/test_practracker.sh.

ex0:
--- /tmp/pracktracker.test.y7DQvj/ex0-received.txt	2019-12-10 10:04:32.845275565 -0500
+++ scripts/maint/practracker/testdata/ex0-expected.txt	2019-12-10 09:52:26.505312737 -0500
@@ -0,0 +1,11 @@
+problem file-size a.c 41
+problem include-count a.c 6
+problem function-size a.c:i_am_a_function() 9
+problem function-size a.c:another_function() 12
+problem dependency-violation a.c 4
+problem file-size b.c 15
+problem function-size b.c:foo() 4
+problem function-size b.c:bar() 5
+problem file-size header.h 8
+problem include-count header.h 4
+problem dependency-violation header.h 3
FAILED

Nick asked me to describe my python situation:

python --version
Python 2.7.17

I have python 2.7, 3.7 and 3.8 installed.

comment:4 Changed 10 months ago by teor

I think this might be a separate bug?

Is there any stderr output?
test_practracker.sh doesn't capture stderr in ex0-received.txt. But it should, so we get better diagnostics.

comment:5 in reply to:  description Changed 10 months ago by teor

Replying to nickm:

Both of these problems are solved if we remove check-practracker-unit-tests, I believe, since test_practracker.sh is already listed in TESTSCRIPTS, so we don't acutally need to have it in check-local as we thought in #32609

I pushed a commit to master, which removes this redundant, broken test. It should fix this CI issue.

comment:6 in reply to:  3 ; Changed 10 months ago by teor

Summary: check-local tries to run test_practracker.sh even if we have no pythontest_practracker.sh python issues

Replying to dgoulet:

So I get this when running scripts/maint/practracker/test_practracker.sh.

ex0:
--- /tmp/pracktracker.test.y7DQvj/ex0-received.txt	2019-12-10 10:04:32.845275565 -0500
+++ scripts/maint/practracker/testdata/ex0-expected.txt	2019-12-10 09:52:26.505312737 -0500
@@ -0,0 +1,11 @@
+problem file-size a.c 41
+problem include-count a.c 6
+problem function-size a.c:i_am_a_function() 9
+problem function-size a.c:another_function() 12
+problem dependency-violation a.c 4
+problem file-size b.c 15
+problem function-size b.c:foo() 4
+problem function-size b.c:bar() 5
+problem file-size header.h 8
+problem include-count header.h 4
+problem dependency-violation header.h 3
FAILED

Nick asked me to describe my python situation:

python --version
Python 2.7.17

I have python 2.7, 3.7 and 3.8 installed.

Maybe practracker is broken with one of your python versions? Or maybe it produces different output?

Can you run scripts/maint/practracker/test_practracker.sh with this PR:

comment:7 Changed 10 months ago by teor

Status: assignedneeds_review

comment:8 Changed 10 months ago by teor

Actual Points: 0.2
Keywords: tor-ci-fixed-fail added; tor-ci-fail removed
Reviewer: dgoulet

comment:9 in reply to:  6 Changed 10 months ago by dgoulet

Status: needs_reviewneeds_information

Replying to teor:

Can you run scripts/maint/practracker/test_practracker.sh with this PR:

Same result... :S

comment:10 Changed 10 months ago by teor

Status: needs_informationneeds_revision

That's really strange. Either the OS or the Python process should produce some kind of output. But you're getting an empty file.

Can you please run:

  • "$PYTHON" --version
  • "${PYTHON:-python}" "scripts/maint/practracker/practracker.py"

I also have to revise this patch, because it failed on Windows:
https://ci.appveyor.com/project/teor2345/tor/builds/29486238

And I want to fix the script so we print something if run_practracker fails.
(Or we should set -e in the script.)

comment:11 Changed 10 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added

comment:12 Changed 9 months ago by nickm

Keywords: 043-should added

comment:13 in reply to:  10 Changed 9 months ago by dgoulet

Replying to teor:

That's really strange. Either the OS or the Python process should produce some kind of output. But you're getting an empty file.

Can you please run:

I assume that by $PYTHON you mean "my python" because it is just not set.

  • "$PYTHON" --version

Python 2.7.17

  • "${PYTHON:-python}" "scripts/maint/practracker/practracker.py"

Does nothing. No output no nothing.

Last edited 9 months ago by dgoulet (previous) (diff)

comment:14 Changed 9 months ago by dgoulet

TOR_DISABLE_PRACTRACKER=1 was the culprit....

Without it, I get a stacktrace with python2 and works with python3. But I assume python3 is what is used anyway so seems all good on my side.

comment:15 Changed 9 months ago by nickm

Status: needs_revisionneeds_review

Here is a small patch to never set TOR_DISABLE_PRACTRACKER in the test:

Branch: no_skip_test_practracker_042
PR: https://github.com/torproject/tor/pull/1658

comment:16 in reply to:  15 ; Changed 9 months ago by catalyst

Replying to nickm:

Here is a small patch to never set TOR_DISABLE_PRACTRACKER in the test:

Branch: no_skip_test_practracker_042
PR: https://github.com/torproject/tor/pull/1658

Looks reasonable at first glance. Maybe we should also make practracker.py write a notice or warning to stderr when it exits early due to TOR_DISABLE_PRACTRACKER, to make this sort of situation easier to troubleshoot in the future?

comment:17 in reply to:  16 Changed 9 months ago by teor

Actual Points: 0.20.5

Replying to catalyst:

Replying to nickm:

Here is a small patch to never set TOR_DISABLE_PRACTRACKER in the test:

Branch: no_skip_test_practracker_042
PR: https://github.com/torproject/tor/pull/1658

Looks reasonable at first glance. Maybe we should also make practracker.py write a notice or warning to stderr when it exits early due to TOR_DISABLE_PRACTRACKER, to make this sort of situation easier to troubleshoot in the future?

I merged nickm's PR 1658 to 0.4.2 and later, and added catalyst's suggested stderr notice.

I also fixed up my PR for master, which improves the practracker tests:

My PR makes the tests:

  • capture stderr output (including exceptions)
  • canonicalise paths, so that logged paths are the same, even in out-of-tree builds
  • fail the test if any process exits unexpectedly
  • check the practracker exit status as part of the tests

I think dgoulet is right, and we shouldn't worry about Python 2 failures. (Unless someone really wants to fix them.) I updated our supported platforms policy to include Python 3:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/SupportedPlatforms#Python

Edit: Supported Python 3 versions only, canonical implementation

Last edited 9 months ago by teor (previous) (diff)

comment:18 Changed 9 months ago by dgoulet

Status: needs_reviewmerge_ready

So I confirm with teor's branch (PR 1599) that it is working fine with my setup.

comment:19 Changed 9 months ago by teor

Merged to master with #32778.

comment:20 Changed 9 months ago by teor

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.