Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip specs failing on TruffleRuby #121

Merged

Conversation

andrykonchin
Copy link
Contributor

@andrykonchin andrykonchin commented May 9, 2024

Some pattern matching specs are failing on the recent TruffleRuby release 24.0. We will investigate the issues - some of them are known. So the failing specs are temporary disabled on TruffleRuby.

@andrykonchin andrykonchin changed the title Skip failing on TruffleRuby specs Skip specs failing on TruffleRuby May 9, 2024
@andrykonchin andrykonchin force-pushed the ak/skip-failing-specs-on-truffleruby branch from 6d23b9c to 9b26718 Compare May 9, 2024 12:48
@andrykonchin andrykonchin force-pushed the ak/skip-failing-specs-on-truffleruby branch from c4a6c1a to 70e66fd Compare May 9, 2024 13:20
assert_block do
case {}
case C.new({})
in {}
C.keys == nil
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it's a bug in https://github.com/ruby/ruby/blob/master/test/ruby/test_pattern_matching.rb that was hidden - C.keys returned keys captured in some previous test case, that by chance matches C.new({}) and captures the nil value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Yeah, having case {} in and checking for C.keys doesn't make any sense here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix in the Ruby repo - ruby/ruby#10757

Copy link
Collaborator

@palkan palkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

One thing: could you please add links to the corresponding TruffleRuby repo issues where we skip it? (If any)

I check them from time to time to see if we can unskip the tests back.

assert_block do
case {}
case C.new({})
in {}
C.keys == nil
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Yeah, having case {} in and checking for C.keys doesn't make any sense here

@andrykonchin andrykonchin marked this pull request as ready for review May 9, 2024 14:07
@andrykonchin
Copy link
Contributor Author

There are no issues for not supported pattern matching edge cases. But failing ruby/spec and MRI tests are tracked in plain text files:

Would you like to have these links added (for every skipped test case)?

@@ -722,6 +725,8 @@ def obj.deconstruct; "" end
end

it "accepts a subclass of Array from #deconstruct" do
next skip if RUBY_ENGINE == "truffleruby"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this test case differs a bit from the one in the ruby/spec repository. Not sure if it's done intentionally. Here the test relies on a way how CRuby accesses array elements - with #[] method. The ruby/spec's current version doesn't redefine the #[] method:

    it "accepts a subclass of Array from #deconstruct" do
      obj = Object.new
      def obj.deconstruct
        Class.new(Array).new([0, 1])
      end

      eval(<<~RUBY).should == true
        case obj
        in [1, 2]
          false
        in [0, 1]
          true
        end
      RUBY
    end

This version passes on TruffleRuby but the version with overridden #[] - doesn't.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the change to ruby/spec was made for TruffleRuby: ruby/spec@0e04268

Unfortunately, there are no additional details on that change.

MRI uses #[] (or opt_aref) to access elements returned by #deconstruct. So, you can do smth like this:

class A < Array
  def [](i) = at(-i-1)
end

a = A.new([1, 2])

case a
in [2, 1]
  true
end #=> true

I don't know if it's an intended feature or not; @k-tsj could you please take a look?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@palkan
Yes, it's intentional. If #[] is redefined, it should be called,.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k-tsj Thank you!

@eregon ^ That's what I mentioned yesterday.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'm not sure it's a good idea to have that behavior in Ruby, maybe something to discuss on ruby-core.
Subclassing Array and redefining [] on its own is already clearly a bad idea, so it seems a niche edge case no one should use.

@palkan
Copy link
Collaborator

palkan commented May 9, 2024

Would you like to have these links added (for every skipped test case)?

Let's add the links to the top of the spec file (ruby_pattern_matching_spec.rb).

@andrykonchin
Copy link
Contributor Author

Done

@andrykonchin
Copy link
Contributor Author

Is there anything that should be done in this PR to have it approved?

@palkan
Copy link
Collaborator

palkan commented Jun 11, 2024

@andrykonchin Sorry, missed latest updates. Looks good, thanks!

@palkan palkan merged commit 682dcab into ruby-next:master Jun 11, 2024
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants