Review: ActionPack fix

tfpt review /shelveset:rails;sborde

Fixes a bug in Proc equality comparison which was causing the ActionPack
caching tests to fail
Splits up the Rails test harness files into tests that fail with MRI as
well (which are often issues with the test)
Changes the tests to load specific versions of Rails and other gems.
Without that, you get load errors because of incorrect versions being
loaded
Changed the default of irtests to run the Rails tests. They can be
skipped by using -m (for “minimum”)

Is ActiveRecord supposed to be commented out (irtests.rb#46)? Also, we
should upgrade to testing the latest version of Rails (2.3.5), but it
doesn’t have to be now.

Looks good otherwise,
~js

From: Shri B.
Sent: Tuesday, January 12, 2010 4:14 PM
To: IronRuby External Code R.
Cc: [email protected]
Subject: Review: ActionPack fix

tfpt review /shelveset:rails;sborde

Fixes a bug in Proc equality comparison which was causing the ActionPack
caching tests to fail
Splits up the Rails test harness files into tests that fail with MRI as
well (which are often issues with the test)
Changes the tests to load specific versions of Rails and other gems.
Without that, you get load errors because of incorrect versions being
loaded
Changed the default of irtests to run the Rails tests. They can be
skipped by using -m (for “minimum”)

The ActiveRecord tests in irtests.rb has never been really enabled for
real (it was enabled with --all but no one used --all). The tests do
work with a little bit of manual setup (downloading ironruby-dbi,
creating the two required test databases, etc), but they should be
enabled in irtests only when they are fully automated.

From: Jimmy S.
Sent: Tuesday, January 12, 2010 7:05 PM
To: Shri B.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

Is ActiveRecord supposed to be commented out (irtests.rb#46)? Also, we
should upgrade to testing the latest version of Rails (2.3.5), but it
doesn’t have to be now.

Looks good otherwise,
~js

From: Shri B.
Sent: Tuesday, January 12, 2010 4:14 PM
To: IronRuby External Code R.
Cc: [email protected]
Subject: Review: ActionPack fix

tfpt review /shelveset:rails;sborde

Fixes a bug in Proc equality comparison which was causing the ActionPack
caching tests to fail
Splits up the Rails test harness files into tests that fail with MRI as
well (which are often issues with the test)
Changes the tests to load specific versions of Rails and other gems.
Without that, you get load errors because of incorrect versions being
loaded
Changed the default of irtests to run the Rails tests. They can be
skipped by using -m (for “minimum”)

In generate_test-unit_tags.rb line 39:
Please change

if fault == testcase_faults.last then comma_separator = “”
else comma_separator = “,”
end

to

if fault == testcase_faults.last
comma_separator = “”
else
comma_separator = “,”
end

In utr it looks like you’ve added ~120 blank lines after line 70, can
you double check please? I’d also like a better way than checking the
defined?-ness of RUBY_ENGINE to disable MRI failures (since 1.9 has
RUBY_ENGINE), but I don’t feel too strongly about that.

JD

From: Shri B.
Sent: Wednesday, January 13, 2010 9:42 AM
To: Jimmy S.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

The ActiveRecord tests in irtests.rb has never been really enabled for
real (it was enabled with --all but no one used --all). The tests do
work with a little bit of manual setup (downloading ironruby-dbi,
creating the two required test databases, etc), but they should be
enabled in irtests only when they are fully automated.

From: Jimmy S.
Sent: Tuesday, January 12, 2010 7:05 PM
To: Shri B.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

Is ActiveRecord supposed to be commented out (irtests.rb#46)? Also, we
should upgrade to testing the latest version of Rails (2.3.5), but it
doesn’t have to be now.

Looks good otherwise,
~js

From: Shri B.
Sent: Tuesday, January 12, 2010 4:14 PM
To: IronRuby External Code R.
Cc: [email protected]
Subject: Review: ActionPack fix

tfpt review /shelveset:rails;sborde

Fixes a bug in Proc equality comparison which was causing the ActionPack
caching tests to fail
Splits up the Rails test harness files into tests that fail with MRI as
well (which are often issues with the test)
Changes the tests to load specific versions of Rails and other gems.
Without that, you get load errors because of incorrect versions being
loaded
Changed the default of irtests to run the Rails tests. They can be
skipped by using -m (for “minimum”)

It looks like Proc equality is simply a reference equality
ReferenceEquals(self, other)with an exception of an empty block for
which the scopes seem to be compared. Or do you have a counterexample?

def foo
a = Proc.new { |x| }
b = lambda &a
c = Proc.new &a
d = Proc.new { |x| }

return a,b,c,d
end

x = foo
y = foo

p x[0] == y[0]
p x[1] == y[1]
p x[2] == y[2]

p x[0] == x[1]
p x[0] == x[2]
p x[0] == x[3]

p x[0].object_id
p x[1].object_id
p x[2].object_id
p x[3].object_id

a = Proc.new {}
b = Proc.new {}
def bar
Proc.new {}
end

p a == b, a.object_id == b.object_id, a == bar, bar == bar

Ruby 1.8:
false
false
false
false
true
false
29077332
29077320
29077332
29077308
true
false
false
false

Tomas

From: Jim D.
Sent: Wednesday, January 13, 2010 9:54 AM
To: Shri B.; Jimmy S.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

In generate_test-unit_tags.rb line 39:
Please change

if fault == testcase_faults.last then comma_separator = “”
else comma_separator = “,”
end

to

if fault == testcase_faults.last
comma_separator = “”
else
comma_separator = “,”
end

In utr it looks like you’ve added ~120 blank lines after line 70, can
you double check please? I’d also like a better way than checking the
defined?-ness of RUBY_ENGINE to disable MRI failures (since 1.9 has
RUBY_ENGINE), but I don’t feel too strongly about that.

JD

From: Shri B.
Sent: Wednesday, January 13, 2010 9:42 AM
To: Jimmy S.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

The ActiveRecord tests in irtests.rb has never been really enabled for
real (it was enabled with --all but no one used --all). The tests do
work with a little bit of manual setup (downloading ironruby-dbi,
creating the two required test databases, etc), but they should be
enabled in irtests only when they are fully automated.

From: Jimmy S.
Sent: Tuesday, January 12, 2010 7:05 PM
To: Shri B.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

Is ActiveRecord supposed to be commented out (irtests.rb#46)? Also, we
should upgrade to testing the latest version of Rails (2.3.5), but it
doesn’t have to be now.

Looks good otherwise,
~js

From: Shri B.
Sent: Tuesday, January 12, 2010 4:14 PM
To: IronRuby External Code R.
Cc: [email protected]
Subject: Review: ActionPack fix

tfpt review /shelveset:rails;sborde

Fixes a bug in Proc equality comparison which was causing the ActionPack
caching tests to fail
Splits up the Rails test harness files into tests that fail with MRI as
well (which are often issues with the test)
Changes the tests to load specific versions of Rails and other gems.
Without that, you get load errors because of incorrect versions being
loaded
Changed the default of irtests to run the Rails tests. They can be
skipped by using -m (for “minimum”)

The examples in
Merlin\External.LCA_RESTRICTED\Languages\IronRuby\mspec\rubyspec\core\proc\shared\equal.rb
would not seem to work with ReferenceEquals

From: Tomas M.
Sent: Wednesday, January 13, 2010 10:12 AM
To: Jim D.; Shri B.; Jimmy S.; IronRuby External Code
Reviewers
Cc: [email protected]
Subject: RE: Review: ActionPack fix

It looks like Proc equality is simply a reference equality
ReferenceEquals(self, other)with an exception of an empty block for
which the scopes seem to be compared. Or do you have a counterexample?

def foo
a = Proc.new { |x| }
b = lambda &a
c = Proc.new &a
d = Proc.new { |x| }

return a,b,c,d
end

x = foo
y = foo

p x[0] == y[0]
p x[1] == y[1]
p x[2] == y[2]

p x[0] == x[1]
p x[0] == x[2]
p x[0] == x[3]

p x[0].object_id
p x[1].object_id
p x[2].object_id
p x[3].object_id

a = Proc.new {}
b = Proc.new {}
def bar
Proc.new {}
end

p a == b, a.object_id == b.object_id, a == bar, bar == bar

Ruby 1.8:
false
false
false
false
true
false
29077332
29077320
29077332
29077308
true
false
false
false

Tomas

From: Jim D.
Sent: Wednesday, January 13, 2010 9:54 AM
To: Shri B.; Jimmy S.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

In generate_test-unit_tags.rb line 39:
Please change

if fault == testcase_faults.last then comma_separator = “”
else comma_separator = “,”
end

to

if fault == testcase_faults.last
comma_separator = “”
else
comma_separator = “,”
end

In utr it looks like you’ve added ~120 blank lines after line 70, can
you double check please? I’d also like a better way than checking the
defined?-ness of RUBY_ENGINE to disable MRI failures (since 1.9 has
RUBY_ENGINE), but I don’t feel too strongly about that.

JD

From: Shri B.
Sent: Wednesday, January 13, 2010 9:42 AM
To: Jimmy S.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

The ActiveRecord tests in irtests.rb has never been really enabled for
real (it was enabled with --all but no one used --all). The tests do
work with a little bit of manual setup (downloading ironruby-dbi,
creating the two required test databases, etc), but they should be
enabled in irtests only when they are fully automated.

From: Jimmy S.
Sent: Tuesday, January 12, 2010 7:05 PM
To: Shri B.; IronRuby External Code R.
Cc: [email protected]
Subject: RE: Review: ActionPack fix

Is ActiveRecord supposed to be commented out (irtests.rb#46)? Also, we
should upgrade to testing the latest version of Rails (2.3.5), but it
doesn’t have to be now.

Looks good otherwise,
~js

From: Shri B.
Sent: Tuesday, January 12, 2010 4:14 PM
To: IronRuby External Code R.
Cc: [email protected]
Subject: Review: ActionPack fix

tfpt review /shelveset:rails;sborde

Fixes a bug in Proc equality comparison which was causing the ActionPack
caching tests to fail
Splits up the Rails test harness files into tests that fail with MRI as
well (which are often issues with the test)
Changes the tests to load specific versions of Rails and other gems.
Without that, you get load errors because of incorrect versions being
loaded
Changed the default of irtests to run the Rails tests. They can be
skipped by using -m (for “minimum”)