Code Review: mspec-12

Ruby IronRuby
1

tfpt review “/shelveset:mspec-12;REDMOND\jflam”

Fixes to enable RubySpecs running in SNAP

  • Implementation of Kernel#exec
  • Fixes bugs in Kernel#exit! - calling incorrect overload
  • Implementation of Kernel#printf
  • Fixes bug in setting visibility of methods defined through attr_*
    methods (now public)
  • Adds overloads for File#open to handle name, modenum case and fixes
    a big in how IO#open was delegating to File constructors
  • Defines a FileTestOps class (does nothing right now)
  • Adds a Process#euid method that always returns 0 (need to understand
    whether detecting admin under windows is possible / realistic using this
    method)
  • Adds a RUBY_PATCHLEVEL constant and sets it to 0
  • Fixes bug in exit n where we were failing to return n as exit code
  • Added a new RubyTestKey.snk which just contains our public key - in
    preparation for removing transform for SIGNED in to_svn rake task
  • A few baseline exclusion changes for existing specs
2

Test issues:

  • Instead of testing for ENV[“HOME”] with
    unless ENV[“HOME”].nil?
    why not use
    if ENV[“HOME”]
    it’s a little more idiomatic.

  • All of the _spec.rb.txt files have been renamed to _tags.txt. You’ll
    need to fix your changeset for that.

  • The new rakefile doesn’t allow testing methods directly. Any reason?
    Also, I’d prefer to keep the test tasks namespaced for organization
    sake. Can you at least move them into the mspec namespace with the rest
    of the mspec tasks? I’ll remove redundant tasks later.

JD

3

  1. There is some garbage in IronRuby.Tests.csproj.

  2. Kernel#printf:
    It shouldn’t look-up $stdin variable by name. Instead,
    RbExecContext.StandardOutput should be used. Also “write” should be
    invoked dynamically.

$a = []
class << $stdout
def write *args
$a << args
end
end

alias $stdout $foo
printf("%d %d", 1, 2)

class << STDOUT
remove_method :write
end

p $a

class Foo
attr_accessor :initialize
end

p Foo.private_instance_methods(false)
p Foo.public_instance_methods(false)

[“initialize”]
[“initialize=”]

Attribute setter is not private.

  1. Yaml initializers shouldn’t be empty.

Tomas

4

Thanks for the catches.

I’m leaving initialize= as public. I think that it’s likely a bug that
attr_accessor :initialize actually runs (Charlie actually agrees with
this). I’ll leave this as is for the time being and open a bug for us to
resolve down the road.

Yaml.Initializers is empty – strange. I reverted the change, but
there’s currently a bug in how ClassInitGenerator generates the code for
Yaml.Initializers. I believe Oleg manually edited to fix the change?
I’ve also updated the Rakefile to generate the initializers for YAML.

I have no idea where the crap that’s in IronRuby.Tests.csproj came from

  • I’ve reverted that change too.

Thanks,
-John

5

Jim D.:

Test issues:

  • Instead of testing for ENV[“HOME”] with
    unless ENV[“HOME”].nil?
    why not use
    if ENV[“HOME”]
    it’s a little more idiomatic.

Done.

  • All of the _spec.rb.txt files have been renamed to _tags.txt. You’ll
    need to fix your changeset for that.

Done.

  • The new rakefile doesn’t allow testing methods directly. Any reason?
    Also, I’d prefer to keep the test tasks namespaced for organization
    sake. Can you at least move them into the mspec namespace with the rest
    of the mspec tasks? I’ll remove redundant tasks later.

It’s done to keep typing to a minimum. I don’t like having to type more
than I have to (and rake regression is already quite long). I replaced
rake test to keep things short as well. I realize that folks can define
aliases for these, but I’d rather the out of box experience be better
than that.

Thanks,
-John

6

Understood. We do need to watch for a littered namespace though if we
are going to keep stuff top level. That’s part of why Rails moved to
namespaces. What’s your thought on splitting the Rakefile into a
compile.rake and a test.rake, for easier maintenance?

JD

From: John L. (IRONRUBY)
Sent: Thursday, June 26, 2008 8:15 PM
To: Jim D.; [email protected]; IronRuby External Code
Reviewers
Subject: RE: Code Review: mspec-12

Jim D.:

Test issues:

  • Instead of testing for ENV[“HOME”] with
    unless ENV[“HOME”].nil?
    why not use
    if ENV[“HOME”]
    it’s a little more idiomatic.

Done.

  • All of the _spec.rb.txt files have been renamed to _tags.txt. You’ll
    need to fix your changeset for that.

Done.

  • The new rakefile doesn’t allow testing methods directly. Any reason?
    Also, I’d prefer to keep the test tasks namespaced for organization
    sake. Can you at least move them into the mspec namespace with the rest
    of the mspec tasks? I’ll remove redundant tasks later.

It’s done to keep typing to a minimum. I don’t like having to type more
than I have to (and rake regression is already quite long). I replaced
rake test to keep things short as well. I realize that folks can define
aliases for these, but I’d rather the out of box experience be better
than that.

Thanks,
-John