Opened 8 years ago

Closed 16 months ago

#5520 closed task (wontfix)

Test the new bootstrap procedure in Vidalia alpha

Reported by: chiiph Owned by: sebb
Priority: Medium Milestone: Vidalia: 0.3.x
Component: Archived/Vidalia Version:
Severity: Normal Keywords: archived-closed-2018-07-04
Cc: chiiph Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Vidalia 0.3.2-rc has a new implementation for the bootstrap procedure. It'd be nice to have it intensively tested.

Basically, the idea would be to take every stage of the bootstrap procedure and stress it out mainly with "tor dies" in each of them. It also handles other kinds of errors but the timing makes it somehow difficult to test, so trying to make sure each of those erroneous situations are in control would be good.

Child Tickets

Attachments (1)

build.log (37.1 KB) - added by sebb 7 years ago.
test suite build log

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by sebb

here's how i can break the tor startup procedure (it worked each time i tested):
1) setup a relay (tested with non-exit relay), disable automatic tor loading
2) delete "cached-*" files from "data" directory
3) start vidalia, click "start tor"
4) wait until "Loading relay information" (~61%), then click "stop tor" - nothing happens
5) click "stop tor" again, everything looks like it should now ("tor is not running" icon etc), but you can't exit vidalia - "exit" menu entry does not work
tor version : tor-browser-2.3.12-alpha-1_en-US
vidalia : latest alpha build
OS: windows xp sp3
btw. i haven't finished implementing automated tests yet, i hope i can find some time in the following week

comment:2 Changed 7 years ago by sebb

Status: newneeds_review

https://github.com/sebthestampede/vidalia/commit/2f629bd0c558544285e413a1ff9e084078be1d13
branch: 5520_bootstrap_test

I've reached a point where I can really use a code review, there are few points that I'm not sure of:

  • I'm new to CMake, so it'd be nice if someone more experienced verifies if the test code is integrated correctly into the project
  • there are few places in the main vidalia code I needed to alter in order to make test code working, I've left comments there
  • bootstrap code in vidalia is simplified too, with use of signals / slots (we talked about it some time ago on irc), it looks like it works correctly, I'm curious to know your opinion on this implementation

This is not the final version, its more like I need to know if this is going in good direction, so any hints are welcome.

comment:3 in reply to:  2 Changed 7 years ago by chiiph

Replying to sebb:

It's seems you've been busy :)

I've reached a point where I can really use a code review, there are few points that I'm not sure of:

  • I'm new to CMake, so it'd be nice if someone more experienced verifies if the test code is integrated correctly into the project

I'm not a test infrastructure guru, but here's what I know: there are a lot of ways of handling tests (see CTest, or QTestLib)

Are these what we need for Vidalia? I don't know. So, in terms of beauty, this might not be the best way to handle tests, but it might be good enough for now, or perfect for an application like Vidalia.

I guess you would have to ask yourself this: how hard would it be to add a test for the settings handling? If the answer is "well, quite simple", then we are good.

  • there are few places in the main vidalia code I needed to alter in order to make test code working, I've left comments there
  • bootstrap code in vidalia is simplified too, with use of signals / slots (we talked about it some time ago on irc), it looks like it works correctly, I'm curious to know your opinion on this implementation

Have you written a sort of execution graph for the different stages of the bootstrap procedure and every possible scenario? I'd like to see that (or something similar) for your code (I'm not asking you to open photoshop or anything :), just some text or simply tell me "yes, I've carefully considered every possible scenario, trust me").
I found out doing this a couple of somewhat signal/slot race conditions that I didn't like, and that's why I went with fully controlled async.

This is not the final version, its more like I need to know if this is going in good direction, so any hints are welcome.

I'll take a closer look to the code tomorrow. Either way, great work! Keep it up!

comment:4 Changed 7 years ago by sebb

This testing code uses QtTest framework, so adding new tests is quite straightforward - it requires to add another class to CMakeList and one method call in testMain.cpp.
I don't have execution graph for the bootstrap procedure. Do you remember where exactly we can expect a race condition, maybe you still have your notes left somewhere ?

comment:5 Changed 7 years ago by chiiph

You use QTestLib, but the tests don't get executed as part of the building procedure. Which is fine, but you asked if it was integrated correctly and the answer is "in terms of developing tests it can be integrated even more, but if it tests things correctly I don't have a problem with executing another application after I rebuild Vidalia".

Regarding the execution graph, one that I can remember was the Circuit Established phase, which can be called twice if the signal is emitted just before a certain method was called where circuitEstablished() was invoked.

The other issue I didn't like is that there are a lot of possible signals emitted from TorControl at any given point, and from the MainWindow it's kind of hard to debug certain issues because you don't have a clear idea which methods where executed when. The graph isn't clear from the code, and that was one of my goals when rewriting that part.

That doesn't mean my solution doesn't have problems, the idea with this task was to change the "it looks like it works" to "I'm 99% certain it works, because I have tools to test every possible scenario I can think of". So if your solution falls in the second sentence, then it's good for me. If you don't know, you need to look at the execution graph and make sure your tests have all that covered.

comment:6 Changed 7 years ago by sebb

Status: needs_reviewneeds_revision

Ok, it seems that you've already been there, done that and i'm moving too far away from the original ticket goal. I'll just revert the changes and leave the bootstrap procedure as it is, changing the implementation deserver at least a separate ticket, maybe someday someone will want to play with it more. I don't have problems with it if it works and is maintainable, and I guess there are more important issues i could take care of for the alpha version.
I hope I can update the branch in few days, lately I'm learning the true meaning of the word "deadline", the hard way ;)

comment:8 Changed 7 years ago by sebb

Status: needs_revisionneeds_review

comment:9 Changed 7 years ago by chiiph

Status: needs_reviewneeds_revision

I get this when trying to test it:

make[2]: *** No rule to make target `/Users/chiiph/Code/vidalia/src/vidalia/i18n/vidalia_ar.qm', needed by `src/tests/qrc_vidalia_i18n.cxx'.  Stop.
make[1]: *** [src/tests/CMakeFiles/VidaliaTestSuite.dir/all] Error 2
make: *** [all] Error 2

Changed 7 years ago by sebb

Attachment: build.log added

test suite build log

comment:10 Changed 7 years ago by sebb

Status: needs_revisionneeds_review

can you check if your cmake cache was cleared before compilation ?
I've just tested it with clear build and it compiled ok, log is attached.

comment:11 Changed 7 years ago by chiiph

Status: needs_reviewneeds_revision

I run the following command:

cd vidalia.build/ && rm -rf * && cmake ../vidalia -DSCRIPT_DIR=~/Code/qtscriptgenerator/plugins/script/ -DBUILD_TESTS=1

and it fails the same. Unless there is another cache somewhere that I'm not clearing?

comment:12 Changed 7 years ago by sebb

Status: needs_revisionneeds_review

my fault, i've had cached files in the source directory
branch is updated, it should compile now

comment:13 Changed 7 years ago by chiiph

I'm trying to use this on OSX, and it keeps being stucked with this:
QSYSTEM: BootstrapTestSuite::testConnectDirMirror() Maximum amount of warnings exceeded. Use -maxwarnings to override.

It seems it's running with full debug output, so it outputs every torControl command it executes.

comment:14 Changed 7 years ago by sebb

Status: needs_reviewneeds_revision

sorry to waste your time like this, i haven't tested it after we updated the alpha branch
i'll check it again as soon as i can

comment:15 Changed 7 years ago by sebb

Status: needs_revisionneeds_review

https://github.com/sebthestampede/vidalia/commit/9f922d20ccfd78fc4b222a051c99caed383c5512
branch: 5520_bootstrap_test

now it has improved timeout functionality, so it wont stuck on a single test case again

"i'll check it again as soon as i can" :D

comment:16 Changed 23 months ago by teor

Severity: Normal

Set all open tickets without a severity to "Normal"

comment:17 Changed 16 months ago by teor

Keywords: archived-closed-2018-07-04 added
Resolution: wontfix
Status: needs_reviewclosed

Close all tickets in archived components

Note: See TracTickets for help on using tickets.