Opened 8 months ago

Closed 7 months ago

#32613 closed task (fixed)

Simple tests for checkSpaces.pl

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points: .1
Parent ID: #32522 Points: .1
Reviewer: teor Sponsor:

Description

I'd like to replace checkSpaces.pl with a python solution. But before we can even think about that, we ought to have tests.

Our existing codebase works well to detect false positives in checkSpaces.pl, so for now, we should just check for false negatives.

Child Tickets

Change History (6)

comment:1 Changed 8 months ago by nickm

Actual Points: .1
Status: assignedneeds_review

I have a test as ticket32613 with PR at https://github.com/torproject/tor/pull/1569

This isn't automated, since we only need to run it when we change our checkSpace.pl script.

comment:2 Changed 8 months ago by nickm

Reviewer: teor

comment:3 Changed 8 months ago by teor

Parent ID: #32522

Seems somewhat related to #32522, and possibly conflicting with #32610.

comment:4 in reply to:  1 Changed 8 months ago by teor

Status: needs_reviewneeds_revision

Replying to nickm:

I have a test as ticket32613 with PR at https://github.com/torproject/tor/pull/1569

Looks fine, please fix a typo in a function name.

This isn't automated, since we only need to run it when we change our checkSpace.pl script.

I think we should run it in "make check" (CI), and the commit hook for these reasons:

  • it's fast
  • if the CI environment breaks, it's important to have known-good tests for diagnostics
  • CI helps us make sure that any changes work on Linux and macOS, across a variety of environments
  • we're going to change this script at least three times in the next few months, in #32610, #29226 (perhaps multiple times), and when we replace it with python
  • developers don't usually remember to run extra tests :-)

And skipping Windows is easy, we have plenty of scripts that do that.

comment:5 Changed 7 months ago by nickm

Status: needs_revisionneeds_review

Okay, I've added the test to run in tests, and skip itself if we're on windows. I've added the script and its test-data to our distribution. It needs a squash, but it merges cleanly for me.

comment:6 Changed 7 months ago by teor

Resolution: fixed
Status: needs_reviewclosed

Thanks for these changes.

I confirmed that the test passes on macOS and Linux, and is skipped on Windows.

Merged to master.

I'll also add this check to the pre-commit hook, once CI passes.

Note: See TracTickets for help on using tickets.