Opened 7 years ago

Closed 6 years ago

#6950 closed task (implemented)

Write tests for Onionoo front-end

Reported by: gsathya Owned by: gsathya
Priority: High Milestone:
Component: Metrics/Onionoo Version:
Severity: Keywords:
Cc: karsten, atagar Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gsathya)

write tests for Pyonionoo

Child Tickets

Change History (27)

comment:1 Changed 7 years ago by karsten

Cc: karsten added

I have been thinking the same. Having a set of tests for Pyonionoo might help us figure out if it provides the same functionality as Onionoo.

How about we add integration tests to Pyonionoo that do the following:

  1. Optionally, start a Pyonionoo front-end with a given summary file (and later details, bandwidth, and weights files). This step is optional, because we could also start a Java Onionoo for the tests. After this step, we assume there is a Pyonionoo or Onionoo server listening on a local port.
  2. Run a couple of tests, each of which sends a (possibly invalid) request and validates the returned result.
  3. Shut down the Pyonionoo server if one was started in step 1.

Maybe Pyonionoo could reuse some of stem's testing harness?

I'd want to help out with finding good test data and test cases.

comment:2 in reply to:  1 Changed 7 years ago by gsathya

Replying to karsten:

I have been thinking the same. Having a set of tests for Pyonionoo might help us figure out if it provides the same functionality as Onionoo.

How about we add integration tests to Pyonionoo that do the following:

  1. Optionally, start a Pyonionoo front-end with a given summary file (and later details, bandwidth, and weights files). This step is optional, because we could also start a Java Onionoo for the tests. After this step, we assume there is a Pyonionoo or Onionoo server listening on a local port.
  2. Run a couple of tests, each of which sends a (possibly invalid) request and validates the returned result.
  3. Shut down the Pyonionoo server if one was started in step 1.

Sounds good.

Maybe Pyonionoo could reuse some of stem's testing harness?

Yeah, it'd be great if we can do this. I'll try to see if this is possible.

I'd want to help out with finding good test data and test cases.

Awesome! I was thinking of finishing up with the features first and then start writing the tests, but I'd be ok with writing tests right now as well :)

comment:3 Changed 7 years ago by gsathya

Component: Onionoopyonionoo
Owner: set to gsathya

comment:4 Changed 7 years ago by karsten

Sounds great! I'm in.

comment:5 Changed 7 years ago by gsathya

Status: newaccepted

WIP - https://github.com/gsathya/pyonionoo/compare/tests

THings to do -

  • add more tests
  • fix the "fixme"s
  • figure a better design than to store responses in a separate file

comment:6 in reply to:  5 Changed 7 years ago by karsten

Replying to gsathya:

WIP - https://github.com/gsathya/pyonionoo/compare/tests

THings to do -

  • add more tests
  • fix the "fixme"s
  • figure a better design than to store responses in a separate file

I had a quick look at the branch, and my first feedback is similar to your third TODO item: comparing returned JSON document strings for equality is probably a poor plan. This is going to fail with Java Onionoo for whitespace reasons and different orderings of dictionary elements (neither of which affect the JSON content). Also, these strings are going to be hard to maintain for future tests. We should rather check the result content for given properties, e.g., number of contained bridges/relays, whether the result contains a given relay or bridge fingerprint, whether there are relays with the running bit even though we didn't ask for them, etc. These checks can be more detailed in the first test or two, and further tests can only check the things we expect to have changed. Maybe there should be helper methods for testing a given property of a returned document, so that the actual test cases can stay small.

I'm happy to help with adding more tests, but I'd rather wait until there's a framework to check response properties, or we'll have to rewrite tests once that framework is there.

comment:7 Changed 7 years ago by karsten

Priority: normalmajor

This ticket is blocking deployment of the pyonionoo front-end, which is blocking extensions of the RESTful API (e.g., #5249, #5255, #6283, #6509). Raising priority to major. I'm starting to write tests for Onionoo now which we can then integrate into pyonionoo's testing framework once it materializes.

comment:8 Changed 7 years ago by gsathya

I've written the scaffolding code for this(will push soonish) -- stuff like is_flag_available(flag), no_of_relays(data), is_fp_available(data), etc.

I'm not sure how the tests should look though. Can you give me an example?

comment:9 in reply to:  8 Changed 7 years ago by karsten

Replying to gsathya:

I've written the scaffolding code for this(will push soonish) -- stuff like is_flag_available(flag), no_of_relays(data), is_fp_available(data), etc.

Nice!

I'm not sure how the tests should look though. Can you give me an example?

On a very high-level perspective I want to test something like this: "If you ask for all summary documents of type bridge (meaning that only bridges are returned) and type relay (meaning that only relays are returned), does it return neither of them as expected?" FYI, Onionoo does not pass this test as I just found out... ;) So, in order to write this test, I don't want to mess with the URL, nor do I want to parse any JSON. I want to use some existing code that I tell to request summary documents, add a "type" parameter with value "bridge" and another "type" parameter with value "relay", and then make sure that the result contains 0 relays and 0 bridges.

There may be lower-level tests where I want to see what happens if I pass a parameter "tpye" with a typo in it, or where I mistakenly replace the "=" between parameter name and value with something else, or where I write the name of an HTTP header wrong, or where the HTTP header is entirely messed up, or whatever. So, we need convenience methods to write short tests, because there will be many of them, and we need the ability to change test details on a fairly low level.

Maybe this becomes clearer once I have a set of tests ready.

comment:10 Changed 7 years ago by karsten

Here's a start.

1 Unparameterized Tests

  • 1.1 Request all summary documents ("GET summary") and make sure that the result matches all properties we can check: same number of relays/bridges as in the test data, relays_published and bridges_published as expected, all nicknames 1-19 alphanumerical characters and as in example data, all fingerprints and hashed fingerprints as in example data and 40 upper-case hex characters, all IP addresses correct with IPv6 hex characters being lower-case (TODO still needs to be specified!), running fields as in example data, no additional fields that are not specified. If there are unspecified fields in the response, that's not an error, but it means we should write tests for them, so it's okay to make the test fail in those cases. This is going to be a long test case with a short request part and a long evaluation part.
  • 1.2 Request all detail documents ("GET details") and test the same things as in 1.1, but for all details fields. Limit to details of a single relay and a single bridge. This is going to be an even longer test case.
  • 1.3 Request all bandwidth documents ("GET bandwidth") and test the same things as in 1.2, but for bandwidth fields. Limit to single relay and single bridge.
  • 1.4 Request all weights documents ("GET weights") and test the same things as in 1.3, but for weights fields.Limit to single relay and single bridge.
  • 1.5 Request a non-existant document type, e.g., "GET doesnotexist", and make sure to receive a 404 response code.
  • 1.6 Request all summary documents, but with header "Accept-Encoding: gzip". The result may or may not be gzip-compressed; both cases are acceptable. But make sure that the result is correct and the server does not fail because of setting this header.
  • 1.7 Request all summary documents, but with an unknown encoding header, e.g., "Accept-Encoding: doesnotexist". Find out what the typical behavior is. Probably an error response code?
  • 1.8 Request all summary documents with a "If-Modified-Since" header far in the past, e.g., in the mid 1980's. The result should be the same as when this header is absent. Only check something trivial, like number of contained relays.
  • 1.9 Request all summary documents with a "If-Modified-Since" header far in the future, e.g., in the mid 2030's. The result should be an empty response.
  • 1.10 Request all summary documents, extract the "Last-Modified" header if present, and request summary documents again with "If-Modified-Since" set to that timestamp. The second result should be empty.
  • 1.11 Request all summary documents, but write summary in upper-case: "GET SUMMARY". Make sure to receive a 404 response code.
  • 1.12--1.xx Request an unknown document type with special characters in it, e.g., space, percent, etc.

2 Filtering parameter tests

[...]

3 Ordering and limiting parameter tests

[...]

comment:11 Changed 7 years ago by karsten

Here's more:

2 Filtering parameter tests

  • 2.1 Request all summaries of type "relay" and make sure that the response has the expected number of relays and no bridges.
  • 2.2 Same as 2.1, but with type "bridge".
  • 2.3 Request all summaries of non-existant type "bridgerelay" and make sure that the result is the expected error code.
  • 2.4 Request all summaries of type "relay" and of type "bridge". Make sure the result contains neither relays nor bridges, because multiple parameters should be handled with a logical AND. (TODO This may need some clarification in the spec.)
  • 2.5 Add a "type" parameter with no "=" and no value to the request. This should return an error code.
  • 2.6 Request all summaries and specify "type=relay" twice in the same request. Make sure that the response is the same as in 2.1.
  • 2.7 Write the parameter name in upper-case letters as "TYPE=relay", and make sure the response is, erm, well, what is it supposed to be?
  • 2.8 Write the parameter value in upper-case letters as "type=RELAY", and make sure the response is as it's supposed to be. See 2.7.
  • 2.9--2.16 Same as 2.1--2.8, but with "running" parameter.
  • 2.17 Search relay by fully specified nickname that is unique in the example data, and make sure that the result contains that relay and nothing else.
  • 2.18 Search relay by non-existant nickname and make sure the result is empty.
  • 2.19 Search relay with a unique, long nickname, but only from the second nickname character on. Result should be empty.
  • 2.20 Search relay by specifying only part of its nickname.
  • 2.21 Search relay by single first nickname character.
  • 2.22 Search existing relay by nickname in all upper-/lower-case, and make sure it's still contained in the result.
  • 2.23 Search relay by $-prefixed, full fingerprint, and make sure it's contained in the result with no other relays or bridges.
  • 2.24 Same as 2.23, but without $-prefixing the full fingerprint.
  • 2.25 Same as 2.23, but with $-prefixed first 39 hex characters of the fingerprint (lower-case).
  • 2.26 Same as 2.25, but with hex-characters all being upper-case.
  • 2.27 Same as 2.25, but without $-prefixing the first 39 lower-case hex characters.
  • 2.28 Search relay by hashed fingerprint, $-prefixed and all 40 characters. Relay should be found.
  • 2.29 Search relay by first 39 $-prefixed hex characters of hashed fingerprint. Relay should be found.
  • 2.30 Search relay by $-prefixed fingerprint consisting of 41 characters, with the first 40 being an existing relay. Should result in error.
  • 2.31 Search relay by fully specified IPv4 address.
  • 2.32 Search relay by its /24 IPv4 network.
  • 2.33 Search relay by its fully specified IPv4 address that it uses for exiting, not as its OR address.
  • 2.34 Same as 2.33, but by searching for the /24 IPv4 network.
  • 2.35 Search relay by fully specified IPv6 address.
  • 2.36 Search relay by its /64 IPv6 network. (TODO This may or may not work in Onionoo.)
  • 2.37 Search relay by fully specified IPv6 address, but without omitting any 0's in the address.
  • 2.38 Search relay by fully specified IPv6 address with hex characters being upper-case.
  • 2.39 Search relay by illegal IPv6 address, e.g., three subsequent colons, more than 8 colon-separated parts, or more than 4 hex characters per part. (TODO This is likely going to fail in Onionoo.)
  • 2.40--2.45 Same as 2.17--2.22, but for bridges.
  • 2.46--2.52 Same as 2.23--2.29, but for bridges.
  • 2.53 Search relays, but include illegal character in search parameter value. Legal characters are "$[0-9][a-z][A-Z].:".
  • 2.54 Same as search cases with fully specified fingerprint, but passed to lookup parameter. This includes upper-/lower-case, existing/non-existing relay/bridge, 39/41 hex characters, $-prefixing (not allowed here), non-hashed/hashed fingerprint, etc.
  • 2.55 Search for relays by country code with a lower-letter country code that is known to result in at least one relays. Make sure the expected relay(s) is/are contained in the response, and make sure there are no bridges in the result.
  • 2.56 Search for relays by country code using an existing country code that has no relays.
  • 2.57 Same as 2.56, but using a non-existing country code.
  • 2.58 Same as 2.56, but using an upper-case country code. Result should be the same.
  • 2.59 Search by three-letter country code.
  • 2.60 Search by one-letter country code.
  • 2.61 Search by country code containing number, e.g., A1 or A2. Should return relays if they are in the test data (make sure there's at least one).
  • 2.62 Search for relays using two different country codes. Should result in empty response.

comment:12 Changed 7 years ago by karsten

And the rest:

3 Ordering and limiting parameter tests

  • 3.1 Request all details documents ordered by consensus weight in ascending order (default). Make sure the ordering is correct. Also make sure that relays without consensus weight and bridges are appended to the end in no specific ordering.
  • 3.2 Same as 3.1, but in descending order. Similarly, make sure that relays without consensus weight and bridges are still appended to the end in no specific ordering.
  • 3.3 Specify the "order" parameter twice. The result is yet unspecified. What should it be? Probably error.
  • 3.4 Specify "consensus_weight,-consensus_weight" as parameter value for "order". The result should be an error.
  • 3.5 Specify "consensus_weight,nickname" as parameter value for "order", which should also be an error (because nickname is not listed as possible field for ordering).
  • 3.6 Specify "CONSENSUS_WEIGHT" as parameter. Unclear if it is and/or should be permitted. Probably should be allowed.
  • 3.7 Request all summary documents ordered by consensus weight in ascending order and make sure they're also ordered correctly. May need to hard-code ordering for example data, or request details documents before and memorize the ordering.
  • 3.8 Request summary documents with an offset of 1 and compare to result without offset. The first relay (or bridge?!) should be gone. (TODO: Specify whether relays or bridges are skipped first.)
  • 3.9 Request summary documents with an offset greater than or equal to the expected number of relays and bridges. The result should be empty.
  • 3.10 Request summary documents with a negative offset. The result should probably be an error code. TODO Specify what should happen.
  • 3.11 Specify a non-numeric offset value. Should result in error.
  • 3.12 Specify offset and order together, e.g., skipping the relay with lowest consensus weight and making sure that the relay with second-lowest consensus weight is first in the result.
  • 3.13 Request summary documents and limit them to 1 result. TODO Specify whether relays or bridges are the first to be cut off.
  • 3.14 Set a negative limit; should result in error.
  • 3.15 Set a limit that's great than or equal to the expected number of relays and bridges, and make sure that all of them are returned.
  • 3.16 Combine order by consensus weight, offset 1, and limit 1 to make sure we get the relay with second-lowest consensus weight.
  • 3.17 Specify a non-numeric limit value. Should result in error.

Feel free to add more cases. Also, feel free to leave out those that don't make sense to you. I'll help implementing them later.

comment:13 Changed 7 years ago by gsathya

Description: modified (diff)

comment:14 Changed 7 years ago by gsathya

Description: modified (diff)

bah typed in the wrong field, edited the description by mistake

comment:15 Changed 7 years ago by karsten

Here's another test case:

  • 3.18 If a query results in X relays and Y bridges, set offset to X-1, X, X+1, X+Y-1, X+Y, X+Y+1 and check that result contains the expected number of relays and bridges. These are really six test cases, not one.

Note that I made a few minor fixes to Onionoo and clarified a few cases in the specification. See my recent onionoo.git commits for details. When implementing the described test cases, don't rely too much on my initial guess what the result should be, but look at the protocol specification.

comment:16 Changed 7 years ago by karsten

gsathya, any news here?

comment:17 Changed 7 years ago by gsathya

Can I get access to the latest output from onionoo? I'm still using the old one I got from you during the dev meeting.

Summary documents don't contain hashed fingerprints do they? (In reference to 1.1)

Here's the initial test - https://github.com/gsathya/pyonionoo/compare/tests2
I'm wondering if I should abstract out some of that code. I'm not sure how or which parts though. Maybe this'll become more obvious once I start adding more tests.

comment:18 in reply to:  17 Changed 7 years ago by karsten

Replying to gsathya:

Can I get access to the latest output from onionoo? I'm still using the old one I got from you during the dev meeting.

Sure. Here's a tarball of the current out/ directory.

Summary documents don't contain hashed fingerprints do they? (In reference to 1.1)

Summary documents contain original relay fingerprints F and hashed bridge fingerprints H(F). They do not, however, contain hashed relay fingerprints H(F) and hashed hashed bridge fingerprints H(H(F)), both of which are supported in the search and lookup parameters.

Here's the initial test - https://github.com/gsathya/pyonionoo/compare/tests2
I'm wondering if I should abstract out some of that code. I'm not sure how or which parts though. Maybe this'll become more obvious once I start adding more tests.

I'll wait until you have some more tests before reviewing, if that's okay for you.

comment:19 Changed 7 years ago by karsten

Cc: atagar added

atagar, you pondered working on pyonionoo. Is that still your plan?

If so, do you have any preferences for choosing a testing framework for Onionoo/pyonionoo? I'd like to start writing tests for Onionoo, and ideally these will be usable for the Python rewrite as well.

Somewhat related, what do you think about keeping the name "Onionoo" and developing the Python rewrite in Onionoo's current Git repo? Similar to (py-)obfsproxy, the Python version will replace the existing version without users noticing, so no need to confuse them with new names/repos. Does that make sense?

comment:20 Changed 7 years ago by atagar

atagar, you pondered working on pyonionoo. Is that still your plan?

I'm pondering three projects...

  • pyonoinoo
  • clean up the arm codebase and finish making a release that uses stem
  • twisted counterpart for the stem's control module

My present plan is to spend a few weeks addressing lingering stem issues, after that we'll see. Each project has a different appeal for me...

  • Pyonionoo would involve improving stem's descriptor support and learning more about databases.
  • Arm would improve stem's controller functionality and is outstanding technical debt for me.
  • Judging by Txtorcon's popularity twisted support is pretty popular. We'll see if the gsoc idea gets any good applications, if not then sinking time here might be a good way for me to improve stem adoption.

If so, do you have any preferences for choosing a testing framework for Onionoo/pyonionoo?

I would probably use python's builtin unittest framework, like stem does.

Somewhat related, what do you think about keeping the name "Onionoo" and developing the Python rewrite in Onionoo's current Git repo?

I like the idea of keeping the name, and can see some advantage for doing the python rewrite within a branch of the onionoo repo. It really doesn't matter at this point though - we can experiment elsewhere then merge in the history later. You're right that if we do our jobs right then no one will even know that the codebase was rewritten. ;)

comment:21 in reply to:  20 ; Changed 7 years ago by gsathya

Replying to atagar:

If so, do you have any preferences for choosing a testing framework for Onionoo/pyonionoo?

I would probably use python's builtin unittest framework, like stem does.

You might want to look at Twisted's testing framework - https://twistedmatrix.com/trac/wiki/TwistedTrial. Also, please let me know if I can help with any part of pyonionoo codebase.

comment:22 in reply to:  21 Changed 7 years ago by karsten

Replying to gsathya:

Replying to atagar:

If so, do you have any preferences for choosing a testing framework for Onionoo/pyonionoo?

I would probably use python's builtin unittest framework, like stem does.

You might want to look at Twisted's testing framework - https://twistedmatrix.com/trac/wiki/TwistedTrial. Also, please let me know if I can help with any part of pyonionoo codebase.

It seems the decision for either testing framework depends on which language pyonionoo will be written in, correct? Or rather, it would seem strange to have a Twisted dependency only for the tests, and using Python's built-in testing framework doesn't make as much sense if the application itself is written in Twisted. I'm fine postponing this decision, in which case I'll hold off writing tests for Onionoo.

Or atagar, do you already know which framework you'll want to use for the Onionoo rewrite?

gsathya, thanks for the offer! I wouldn't mind some assistance with writing the first few test cases once we know which testing framework to use.

comment:23 Changed 7 years ago by atagar

Or atagar, do you already know which framework you'll want to use for the Onionoo rewrite?

I don't. My bias for projects I do is to have minimal dependencies. I should give twisted a try at some point if only to broaden my horizons, though I doubt that I would do it for Pyonionoo.

This might be moot though - again, I'm not yet sure what project I'll be moving on to next. I have precious little time and soon will have even less. Pyonionoo might be too ambitious for me along with a fulltime job.

comment:24 in reply to:  23 ; Changed 6 years ago by karsten

Component: pyonionooOnionoo
Status: acceptedneeds_review

Replying to atagar:

Or atagar, do you already know which framework you'll want to use for the Onionoo rewrite?

I don't. My bias for projects I do is to have minimal dependencies. I should give twisted a try at some point if only to broaden my horizons, though I doubt that I would do it for Pyonionoo.

Makes sense.

This might be moot though - again, I'm not yet sure what project I'll be moving on to next. I have precious little time and soon will have even less. Pyonionoo might be too ambitious for me along with a fulltime job.

That also makes sense. After all, the Java Onionoo is in a pretty good shape, it's just sad that the language prevents people from contributing. But it should be a fun project to rewrite it in Python, not an endless time sink.

Anyway, Onionoo really needs testing. Given that there's no Pyonionoo on the horizon, I decided to write unit tests in Java/JUnit. Want to take a look and give me some feedback? I'm planning to write similar unit tests for the parts of Onionoo that parse descriptors and produce details/bandwidth/weights files that Onionoo later hands out.

Making this an Onionoo task, because that seems more relevant now than Pyonionoo.

comment:25 in reply to:  24 ; Changed 6 years ago by cypherpunks

Replying to karsten:

Anyway, Onionoo really needs testing. Given that there's no Pyonionoo on the horizon, I decided to write unit tests in Java/JUnit. Want to take a look and give me some feedback? I'm planning to write similar unit tests for the parts of Onionoo that parse descriptors and produce details/bandwidth/weights files that Onionoo later hands out.

Why write tests in Java? If you write it in Python, they can be reused in Pyonionoo.

comment:26 in reply to:  25 Changed 6 years ago by karsten

Replying to cypherpunks:

Why write tests in Java? If you write it in Python, they can be reused in Pyonionoo.

Well, these are unit tests, so there was not much of a choice.

comment:27 Changed 6 years ago by karsten

Resolution: implemented
Status: needs_reviewclosed
Summary: write tests for pyonionooWrite tests for Onionoo front-end

Refined unit tests a bit more, so that they can be easily adopted in a future Pyonionoo. Most of the tests contain a test URI and either number and possibly order of expected relays and bridges in the result, or expected error status code.

That concludes the task IMO. Closing as implemented.

Note: See TracTickets for help on using tickets.