Opened 3 years ago

Closed 3 years ago

#11335 closed enhancement (implemented)

Tests for example scripts

Reported by: atagar Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords: testing easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We recently added a scripts section to our examples. We should have unit tests for these. See our tutorial unit tests for an example of how we could do this.

I really dislike the idea of duplicating these scripts in our tests so bonus points if you can figure out a way of having our tests read the docs! This might actually be a decent use of eval...

Child Tickets

Change History (12)

comment:1 in reply to:  description Changed 3 years ago by sambuddhabasu

Does the tutorial unit tests link actually refer to the other link?
https://gitweb.torproject.org/stem.git/tree/test/unit/tutorial.py
Replying to atagar:

We recently added a scripts section to our examples. We should have unit tests for these. See our tutorial unit tests for an example of how we could do this.

I really dislike the idea of duplicating these scripts in our tests so bonus points if you can figure out a way of having our tests read the docs! This might actually be a decent use of eval...

comment:2 Changed 3 years ago by atagar

Yup. GitWeb recently upgraded so links in tickets are no longer correct.

comment:3 Changed 3 years ago by sambuddhabasu

Added a test for example script Link Circuits. Please review my commit https://github.com/sammyshj/stem/commit/c62ef284f3735dea99dce492ca3b4750fa9fb9d

comment:4 Changed 3 years ago by atagar

Hi sambuddhabasu, this is a great start!

mocking.get_protocolinfo_response()

What you're doing here is a hack and liable to break (... in fact, I'm a little surprised it runs right now). The get_protocolinfo_response() provides a ControlMessage like what Tor provides in reply to a PROTOCOLINFO request. You're essentially making the wrong type of object, then calling setattr() to give it an address attribute so you can use it like a RouterStatusEntry.

I just gave get_protocolinfo_response() as an example, I didn't expect you to use it directly. Take a look at what the Controller methods return, then look for corresponding types in the mock module.

Cheers! -Damian

comment:5 Changed 3 years ago by sambuddhabasu

Hi atagar,

Thanks for pointing that out. Now, the RouterStatusEntry and CircuitEvent classes are put into use.
The commit can be seen here
https://github.com/sammyshj/stem/commit/ad94cd615e1078c0654b899d601896ebf672d8e3
Please review it

Thanks
Sambuddha

comment:6 Changed 3 years ago by atagar

Looks great! Here's some suggestions that could make the code a bit nicer...


content = get_router_status_entry_v3({'r': r_line}, content = True)
entry = RouterStatusEntryV3(content)

Why not simply drop 'content = True' so the function provides you a RouterStatusEntryV3?


def side_effect_get_network_status(relay, default):
  values = {
    path_1[0]: _get_router_status("173.209.180.61"),
    path_2[0]: _get_router_status("87.238.194.176"),
    path_3[0]: _get_router_status("109.163.234.10"),
    path_4[0]: _get_router_status("46.165.197.96"),
    path_5[0]: _get_router_status("96.47.226.20"),
    path_6[0]: _get_router_status("86.59.119.83"),
    path_7[0]: _get_router_status("176.67.169.171")
  }
  return values[relay]

...

controller.get_network_status.side_effect = side_effect_get_network_status

A little trick I like to use for this is...

controller.get_network_status.side_effect = lambda fingerprint, *args: {
  path_1[0]: _get_router_status("173.209.180.61"),
  path_2[0]: _get_router_status("87.238.194.176"),
  path_3[0]: _get_router_status("109.163.234.10"),
  path_4[0]: _get_router_status("46.165.197.96"),
  path_5[0]: _get_router_status("96.47.226.20"),
  path_6[0]: _get_router_status("86.59.119.83"),
  path_7[0]: _get_router_status("176.67.169.171")
}[fingerprint]

circuit_4_path = PATH_CONTENT % (path_1[0], path_1[1], path_2[0], path_2[1], path_3[0], path_3[1])
circuit_6_path = PATH_CONTENT % (path_1[0], path_1[1], path_4[0], path_4[1], path_5[0], path_5[1])
circuit_10_path = PATH_CONTENT % (path_1[0], path_1[1], path_6[0], path_6[1], path_7[0], path_7[1])

circuit_4_content = CIRC_CONTENT % (4, 'BUILT', circuit_4_path, 'GENERAL')
circuit_6_content = CIRC_CONTENT % (6, 'BUILT', circuit_6_path, 'GENERAL')
circuit_10_content = CIRC_CONTENT % (10, 'BUILT', circuit_10_path, 'GENERAL')

circuit_4 = _get_event(circuit_4_content)
circuit_6 = _get_event(circuit_6_content)
circuit_10 = _get_event(circuit_10_content)

It would probably make more sense for _get_event() the CIRC event attributes. That is to say...

circuit_4 = _get_event(4, 'BUILT', hop1, hop2, hop3, 'GENERAL')

comment:7 Changed 3 years ago by sambuddhabasu

The necessary changes have been made which makes the code cleaner. The _get_event function is now smarter and handles the event attributes too.
The commit can be seen at:
https://github.com/sammyshj/stem/commit/26ea4b04b62c4824f27e7e4e0c3716472c93c4bb

comment:8 Changed 3 years ago by atagar

Looks good! I'd be happy to merge this or wait for the other examples. Up to you.

Thanks sambuddhabasu!

comment:9 Changed 3 years ago by sambuddhabasu

I have added the rest of the tests too. Hope they are as expected.
The files changed can be seen here:
https://github.com/sammyshj/stem/pull/2/files
Please let me know what you think about the tests.

comment:10 Changed 3 years ago by atagar

Yikes, this looks fantastic! I won't be able to more thoroughly review it until the weekend but on first glance looks great - nice work!

comment:11 Changed 3 years ago by sambuddhabasu

Good to hear that thanks.
Hoping to hear from you soon then :)

comment:12 Changed 3 years ago by atagar

Resolution: implemented
Status: newclosed

Looks great! Merged with some small revisions. Thanks for the help!

Note: See TracTickets for help on using tickets.