Opened 6 years ago

Closed 3 years ago

#9087 closed enhancement (fixed)

Move network tests out of TorNet.py

Reported by: ln5 Owned by: chobe
Priority: Medium Milestone:
Component: Core Tor/Chutney Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

As Nick points out in #8531, it's far from perfect that new network
tests need to go into TorNet.py.

Maybe read a config file or pick up .py files from a tests/ directory?

Child Tickets

Attachments (3)

chobe-9087.patch (6.5 KB) - added by chobe 5 years ago.
chobe-9087-v2.patch (8.2 KB) - added by chobe 5 years ago.
0001-Move-network-tests-to-separate-Python-files.patch (22.3 KB) - added by cypherpunks 3 years ago.

Download all attachments as: .zip

Change History (12)

Changed 5 years ago by chobe

Attachment: chobe-9087.patch added

comment:1 Changed 5 years ago by chobe

This patch is pretty rudimentary.
It simply scans tests/chutney_tests/* for the command entered (if it doesn't match the basic Network object to the "test" function defined by the py script.

Version 0, edited 5 years ago by chobe (next)

comment:2 Changed 5 years ago by chobe

Status: newneeds_review

Changed 5 years ago by chobe

Attachment: chobe-9087-v2.patch added

comment:3 Changed 5 years ago by chobe

Talked with people in tor-dev and came up with a better solution using __import__
I also changed the directory "tests" to "scripts" because "tests/chutney_tests" sounds ridiculous and "scripts" is a lot more flexible.
Added the function "getTests" and used it whenever possible to make changing the code easier.

comment:4 Changed 3 years ago by nickm

Owner: changed from nickm to chobe
Severity: Normal
Status: needs_reviewassigned

comment:5 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:6 Changed 3 years ago by teor

Status: needs_reviewneeds_revision

This patch modifies code that was also modified in #18826, would you mind rebasing it on the current chutney master?

comment:7 Changed 3 years ago by cypherpunks

Status: needs_revisionneeds_review

I have gone ahead and rebased the patch on the current master. Because the verify function in Tornet.py was extended between the latest patch and now, i reimported the code into the verify script. Assuming the patch is applied in a separate branch, you can verify the code move with

diff -u -w <(git show master:lib/chutney/TorNet.py | sed -n '906,1097p') scripts/chutney_tests/verify.py

which selects the appropriate lines and diffs them with the verify script. As you can see, small changes had to be made for it to work properly.

I also made other small changes which should make the change more robust.

  1. changed the README addition to improve its readability.
  2. used import_module instead of __import__ because it's the recommended way of importing modules according to the official documentation.
  3. detect when the test script has no run_test(network) function and alert the user.

FWIW this implementation takes preference to the test scripts instead of the commands which means that when a test script is called start, configure, etc. it will overwrite the commands. Maybe this should be prevented in the code or documented somewhere. I don't have a strong opinion on either case.

comment:8 in reply to:  7 Changed 3 years ago by teor

Replying to cypherpunks:

I have gone ahead and rebased the patch on the current master. Because the verify function in Tornet.py was extended between the latest patch and now, i reimported the code into the verify script. Assuming the patch is applied in a separate branch, you can verify the code move with

diff -u -w <(git show master:lib/chutney/TorNet.py | sed -n '906,1097p') scripts/chutney_tests/verify.py

which selects the appropriate lines and diffs them with the verify script. As you can see, small changes had to be made for it to work properly.

I also made other small changes which should make the change more robust.

  1. changed the README addition to improve its readability.
  2. used import_module instead of __import__ because it's the recommended way of importing modules according to the official documentation.
  3. detect when the test script has no run_test(network) function and alert the user.

Thanks! This looks great, and will really help us extend chutney.

FWIW this implementation takes preference to the test scripts instead of the commands which means that when a test script is called start, configure, etc. it will overwrite the commands. Maybe this should be prevented in the code or documented somewhere. I don't have a strong opinion on either case.

Let's document it as part of documenting the test script feature. If someone wants to override their own start command, they're an expert, and they can keep all the pieces if it breaks.

comment:9 Changed 3 years ago by teor

Resolution: fixed
Status: needs_reviewclosed

Merged to master as 14bb7b8, with a note about how to call commands, and overriding built-in commands in e20712c.

Note: See TracTickets for help on using tickets.