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.

Sign up for GitHub

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

Jump to bottom

Don't run integration tests on Falcon against TruffleRuby #1839

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Don't run integration tests on Falcon against TruffleRuby #1839

merged 1 commit into from
Nov 15, 2022

Conversation

andrykonchin
Copy link
Contributor

@andrykonchin andrykonchin commented Nov 14, 2022 •
edited
Loading

Hello.

TruffleRuby doesn't support Fiber.set_scheduler method so it will be great not to run tests (temporary) that involve this method call on TruffleRuby.

Right now only integration tests with Falcon rely on this method so here in this PR Falcon server is skipped in tests if Fiber.set_scheduler is not implemented.

Does it make sense?

@dentarg
Copy link
Member

dentarg commented Nov 14, 2022

I guess this is in response to truffleruby-head (truffleruby 23.0.0-dev-a645f1b5) failing at https://github.com/sinatra/sinatra/actions/runs/3447564172/jobs/5753892737#step:5:14, right?

Sure, I guess it makes some sense

How come truffleruby (truffleruby 22.3.0) does not fail at https://github.com/sinatra/sinatra/actions/runs/3447564172/jobs/5753892621#step:5:19? Is falcon behaving differently on Ruby 3.0.x than 3.1.x?

@andrykonchin
Copy link
Contributor Author

I guess this is in response to truffleruby-head (truffleruby 23.0.0-dev-a645f1b5) failing at https://github.com/sinatra/sinatra/actions/runs/3447564172/jobs/5753892737#step:5:14, right?

Yeah, exactly

Is falcon behaving differently on Ruby 3.0.x than 3.1.x?

Good question. I haven't noticed that specs passed on TruffleRuby 22.3.0 (CRuby 3.0).

Indeed, falcon behaves differently on Ruby 3.0 and Ruby 3.1. On Ruby 3.0 falcon depends on async 1.30.3 that doesn't use Fiber.set_scheduler. On Ruby 3.1 falcon depends on async 2.1.1 that depends on Fiber.set_scheduler.

@dentarg
Copy link
Member

dentarg commented Nov 15, 2022 •
edited
Loading

Is it possible to conditionally skip the test (so it is still seen in the test output) instead of deleting falcon from the servers array? Or do we error out too early for that?

@andrykonchin
Copy link
Contributor Author

andrykonchin commented Nov 15, 2022 •
edited
Loading

Yeah, tests fail too early - when a falcon server is run.

It happens before any integration test case is executed when IntegrationHelper is extending a file with tests and an extend_object callback is called:

sinatra/test/integration_helper.rb

Lines 121 to 128 in cafaab9

def self.extend_object(obj)
super
base_port = 5000 + Process.pid % 100
Sinatra::Base.server.each_with_index do |server, index|
Server.run(server, base_port+index)
end
end

We may print some warning (when remove the falcon server from the servers list) to make this skipping more explicit. Will it be helpful?

@dentarg
Copy link
Member

dentarg commented Nov 15, 2022

Sounds good to me

(FYI it is up to @jkowens to merge this as I don't have such access)

@andrykonchin andrykonchin force-pushed the ak/skip-falcon-in-specs branch from 2f0beef to 09ee32f Compare November 15, 2022 17:41
@andrykonchin
Copy link
Contributor Author

@jkowens Could you please take a look and merge if it looks OK?

@andrykonchin andrykonchin force-pushed the ak/skip-falcon-in-specs branch from 09ee32f to 42e3eed Compare November 15, 2022 17:43
@jkowens jkowens merged commit c90f203 into sinatra:master Nov 15, 2022
@andrykonchin andrykonchin deleted the ak/skip-falcon-in-specs branch November 16, 2022 09:43
@andrykonchin
Copy link
Contributor Author

Thank you!

@jkowens
Copy link
Member

jkowens commented Nov 16, 2022

@andrykonchin thanks for the PR 👍

dentarg
dentarg reviewed Dec 24, 2022
View reviewed changes
test/integration_helper.rb
servers = Sinatra::Base.server.dup

# TruffleRuby doesn't support `Fiber.set_scheduler` yet
unless Fiber.respond_to?(:set_scheduler)
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this check also skips falcon tests on MRI Ruby <3.0

$ chruby-exec 2.7.7 -- ruby -e 'p Fiber.respond_to?(:set_scheduler)'
false

falcon has an open requirement on async, async since version 2.0 requires Ruby >=3.1.1 (https://rubygems.org/gems/async/versions/2.0.0). It should be possible to still run these tests on Ruby 2.x, don't you think @andrykonchin? It will just run falcon using async 1.x

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, so probably we should check explicitly for RUBY_ENGINE == "truffleruby" here.
TruffleRuby master has RUBY_VERSION 3.1 but doesn't support Fiber.set_scheduler yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

So:

    if RUBY_ENGINE == "truffleruby" && !Fiber.respond_to?(:set_scheduler)

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've prepared a PR with the fix (#1852).

@andrykonchin andrykonchin mentioned this pull request Dec 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon left review comments

@dentarg dentarg dentarg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andrykonchin @dentarg @jkowens @eregon