Test Review:
classes.rb:
* can you change the get_status_... methods to status_... methods? The extra
get isn't really used in Ruby.
* Did you want to have possibly stale information in the Status class?
* for running thread, why not just do:
def status_of_running_thread
t = Thread.new {loop {}}
Thread.pass until t.status == "run"
status = Status.new t
t.kill
status
end
* critical_should_be_false should use ScratchPad instead of should's in the
method. I would suggest trying to always keep shoulds in the spec itself.
* Can you modify method names and variables to be snake_case instead of
CamelCase?
Alive_spec.rb:
* (new thing I'm going to start doing with new guards) What's the justification
for the compliant_on(:ruby) guard. Why isn't it a tag?
Critical_spec.rb:
* add a before(:each) block and move the ScratchPad.clear to there, then you
can add ScratchPad without thinking about it.
* Extra comment (#end) on line 49
Exit.rb:
* lines 11 and 23: I don't see a reason for checking the value of the threads.
You are already checking the ScratchPad, so you know the execution ran.
Raise_spec.rb:
* You might need to remove the " raises an exception on running thread" tag. I
think I added one when doing dates.
Wakeup.rb:
* Line 38 has an unused Channel
* 1000 times for the deadlock test might be too long for a test. In principle
it's nice for the stress portion, but it seems too long. I'd also like to find
another way than 1.should == 1, but you can check that in and I'll find a way.
JD
-----Original Message-----
From: [email protected]
[mailto:[email protected]] On Behalf Of Tomas Matousek
Sent: Wednesday, January 07, 2009 2:37 PM
To: Shri Borde; [email protected]; IronRuby External Code Reviewers
Subject: Re: [Ironruby-core] Code Review: hangs in core\thread
I would prefer using a field rather than a private auto-property for IsSleeping.
Other than that, changes in ThreadOps look good.
Tomas
-----Original Message-----
From: Shri Borde
Sent: Wednesday, January 07, 2009 2:18 PM
To: [email protected]; IronRuby External Code Reviewers
Subject: RE: Code Review: hangs in core\thread
Since no one has reviewed this yet, I have updated the change to factor out the
tests for alive?, inspect, status and stop?
Another product change was that Thread#value should return false if the thread
was killed. Only a thread with an uncaught exception should return nil.
Thanks,
Shri
-----Original Message-----
From: [email protected]
[mailto:[email protected]] On Behalf Of Shri Borde
Sent: Tuesday, January 06, 2009 11:49 PM
To: IronRuby External Code Reviewers
Cc: [email protected]
Subject: [Ironruby-core] Code Review: hangs in core\thread
tfpt review "/shelveset:hangs;REDMOND\sborde"
Comment :
Non-deterministic fixes. I ran all the core\thread specs in a loop in
multiple processes and ran into a few non-deterministic failures. This change
fixes everything I ran into. The only product change is that Thread#wakeup is
not queued up if the target thread is not sleeping. This is the MRI behavior.
Also started using comments in the tag files like this - fails("reason
why"):test name. I will undo this if Jim says that he will be regenerating the
tags file periodically.
_______________________________________________
Ironruby-core mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/ironruby-core
