Opened 7 years ago

Closed 7 years ago

#7295 closed defect (fixed)

Core tests are broken

Reported by: hellais Owned by: hellais
Priority: Very High Milestone:
Component: Archived/Ooni Version:
Severity: Keywords:
Cc: isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Something after commit c12bc4d89ef16d78e2cf2ef22363802ec90022ae broke the core tests.

I believe it has something to do with the changes that have been made to runner.py.

Here is a list of things that I noticed that may or may not be related to this issue, but that are for sure not good practice
(this is all related to ooni/runner.py):

There is code duplication in processTest(obj, config) and processTestOptions.
Looking around processTest appears to no longer be called.

loadTestsAndOptions has become a huge method. I would consider splitting the things that have to do with the legacy API to another place.

The uber deep nesting plague appears to have started to attack parts of the code. In particular https://gitweb.torproject.org/ooni-probe.git/blob/HEAD:/ooni/runner.py#l200, https://gitweb.torproject.org/ooni-probe.git/blob/HEAD:/ooni/runner.py#l238, https://gitweb.torproject.org/ooni-probe.git/blob/HEAD:/ooni/runner.py#l258

If you start nesting beyond one level, you should ask yourself if there is a better way to write it.

We should also not spend any more time on supporting the old API. Every line of code we write to support it is a line of code we have to maintain, that is bad.

Child Tickets

Change History (3)

comment:1 Changed 7 years ago by isis

loadTestsAndOptions was split up into processLegacyTest and processNetTest. see https://gitweb.torproject.org/ooni-probe.git/commitdiff/743e6f57cf72058a253134cce092aad9ea95379a

processTest didn't need to be in there, and it was removed a couple days ago.

The actual bug that is blocking everything, and the reason why I keep restructuring runner and nettest is because the were not handling the creation of a TestCase's Options class, specifically they were not handling subOptions at all. This was blocking the bridge reachability tests from working.

It is enough of a nightmare that I'm expected to have ooni working, and be expected to keep the bridge tests updated with a complete API change three weeks before both deliverables are due. When you said you were thinking of changing the API, I knew this would happen and I thought of making the bridge tests separate from ooni because I didn't want to deal with fixing both. But I really want them to be integrated, and I really want both to work. When you change an API, you're supposed to look at its common use cases and make sure those don't break. The API change broke all of my tests. Now I'm fixing both ooni's API and the bridge reachability tests pretty much by myself, and I have completely missed four nights of sleep this week. I'm exhausted, I have not been eating, I have not paid any attention to any of my friends or family, and I can't handle this much more.

I need my old tests to work, and that means their options being parsed correctly. The new ones need to work too. Please, Arturo, will you help me fix this?

comment:2 Changed 7 years ago by hellais

Status: newneeds_review

I ended up solving this by porting BridgeT to the new API.

We now also have support for advancedOptParameters as you suggested.

We should from now on only use the new API and remove all the references to the old API.

Maintaining two API is a headache and it is optimal to only go for the new one.

comment:3 Changed 7 years ago by hellais

Resolution: fixed
Status: needs_reviewclosed

This has been resolved.

Note: See TracTickets for help on using tickets.