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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only run auto compact tests on platforms that support it #891

Closed
wants to merge 1 commit into from

Conversation

tenderlove
Copy link
Member

Some platforms (ppcle for example) can't support autocompact. Don't run
these tests on non-supported platforms

21a48d9 is causing failures on some CI machines (particularly the ones that don't support compaction). I'm not sure if this is the right way to guard against running the tests 馃槄

Some platforms (ppcle for example) can't support autocompact.  Don't run
these tests on non-supported platforms
@eregon
Copy link
Member

eregon commented Oct 29, 2021

@eregon
Copy link
Member

eregon commented Oct 29, 2021

Would it make sense for these methods to still succeed even on unsupported platforms?
For instance these methods are implemented on TruffleRuby but are just accessors (compaction is always enabled on JVM), similar to a few other GC methods:
https://github.com/oracle/truffleruby/blob/f2d856f50db7e3ac8447fe9edd54a4756b7b7436/src/main/ruby/truffleruby/core/gc.rb#L94-L132

What happens when running those tests on unsupported platforms?

@eregon
Copy link
Member

eregon commented Oct 29, 2021

I think ideally:

  • Either these methods work (but have no effect) on unsupported platforms
  • There is an easy way to test if supported, e.g. GC.respond_to?(:auto_compact=)/GC.auto_compact_supported?

It also might fine to simply call these methods and rescue a well known exception which means unsupported and use skip then.

I would much prefer not depending on a check needing Etc, GC::INTERNAL_CONSTANTS and other details which might change.

@eregon
Copy link
Member

eregon commented Oct 29, 2021

Found the failing log:
http://rubyci.s3.amazonaws.com/ppc64le/ruby-master/log/20211029T170005Z.fail.html.gz


1)
GC.auto_compact can set and get a boolean value ERROR
NotImplementedError: Automatic compaction isn't available on this platform
<internal:gc>:59:in `auto_compact='
/home/chkbuild/chkbuild/tmp/build/20211029T170005Z/ruby/spec/ruby/core/gc/auto_compact_spec.rb:15:in `block (3 levels) in <top (required)>'
/home/chkbuild/chkbuild/tmp/build/20211029T170005Z/ruby/spec/ruby/core/gc/auto_compact_spec.rb:4:in `block in <top (required)>'
/home/chkbuild/chkbuild/tmp/build/20211029T170005Z/ruby/spec/ruby/core/gc/auto_compact_spec.rb:3:in `<top (required)>'

2)
An exception occurred during: after :each
GC.auto_compact can set and get a boolean value ERROR
NotImplementedError: Automatic compaction isn't available on this platform
<internal:gc>:59:in `auto_compact='
/home/chkbuild/chkbuild/tmp/build/20211029T170005Z/ruby/spec/ruby/core/gc/auto_compact_spec.rb:10:in `block (3 levels) in <top (required)>'
/home/chkbuild/chkbuild/tmp/build/20211029T170005Z/ruby/spec/ruby/core/gc/auto_compact_spec.rb:4:in `block in <top (required)>'
/home/chkbuild/chkbuild/tmp/build/20211029T170005Z/ruby/spec/ruby/core/gc/auto_compact_spec.rb:3:in `<top (required)>'

I think it's best to just rescue NotImplementedError and skip those tests in that case then.
We can move the save/restore pattern inside the it to avoid errors in the after.

matzbot pushed a commit to ruby/ruby that referenced this pull request Oct 29, 2021
@eregon
Copy link
Member

eregon commented Oct 29, 2021

@tenderlove Thank you for the PR and making me aware of the issue.
I pushed a fix to CRuby (ruby/ruby@800dad6) and I'll commit it here to to fix rubyci.org.

@eregon eregon closed this Oct 29, 2021
@eregon eregon deleted the guard-compact branch October 29, 2021 19:55
eregon added a commit that referenced this pull request Oct 29, 2021
@tenderlove
Copy link
Member Author

@eregon thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants