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

[CI] Fix up integration stream test for Puma #1887

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

MSP-Greg
Copy link
Contributor

@MSP-Greg MSP-Greg commented Feb 22, 2023

2nd commit fixes timing issue with Puma, adds 0.05 second to sleep, small changes to asserts and time calc

Two commits:

  1. .gitignore - add Bundler items

  2. [CI] Fix up integration stream test

@dentarg
Copy link
Member

dentarg commented Feb 22, 2023

truffleruby failed with

  1) Failure:
IntegrationTest#test_with_puma_streams_0 [/home/runner/work/sinatra/sinatra/test/integration_test.rb:40]:
Expected 1 to be > 1.02.

I guess we can adjust something?

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Feb 22, 2023

Thanks, I saw that, but AFK for a bit. As we've seen in Puma, TruffleRuby can take a bit longer to start a server, due to all the threads.

I thought I'd look at starting Puma with threads '1:4'.

Any thoughts? Also, it might still need a timing adjustment...

@dentarg
Copy link
Member

dentarg commented Feb 22, 2023

Sure, sounds good to lower the Puma thread count in CI (because that's what you're suggesting, right?)

@MSP-Greg
Copy link
Contributor Author

Actually, no. If min_threads is zero, the thread is created when the first request is received, which would increase the time for the test assert that failed. For CI, if min_threads is one, the thread is created as the server boots.

Most tests only submit one request, so the max isn't important...

@dentarg
Copy link
Member

dentarg commented Feb 22, 2023

I see, yeah that makes a lot of sense. Need help changing that? (The test "framework" here is a bit "magic"). I wonder if we can just set an ENV?

@MSP-Greg
Copy link
Contributor Author

Need help changing that?

Nobody can help me!

Sorry, one of those days. See the last & recent commit. It passed in my fork, but not sure if it's an intermittent problem. If it does fail again, that timing criteria should change, possibly by platform/os.

The test "framework" here is a bit "magic"

Anytime servers get spun up, things are always interesting...

@dentarg dentarg merged commit 50b8398 into sinatra:master Feb 22, 2023
@dentarg
Copy link
Member

dentarg commented Feb 22, 2023

Thanks for these changes!

@MSP-Greg MSP-Greg deleted the 00-integration-stream branch February 22, 2023 22:07
@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Feb 23, 2023

@dentarg

I looked at the test system a bit more this evening. Since ping is being called, the last commit could probably be reverted, but it doesn't hurt. Running locally on a fast desktop, I had the following times:

Set1  Set2
0.21  0.21  int1  Ruby MRI
1.14  1.15  int2

0.12  0.12  int1  JRuby
1.25  1.25  int2

0.34  0.32  int1  TruffleRuby 
1.05  1.04  int2

Given the below, hard to say whether any of the limits should change. Actions CI system's 'speed' can vary quite a bit.

assert_operator 1, :>, int1
assert_operator 1, :<, int2

It's interesting that for all tests the sum of int1 & int2 is fairly close to 1.35, which is the total delay in the test (0.1 + 1.25).

EDIT: Re the times for Ruby MRI, I believe the time limit for a 'corked' socket is 200 mS, so the int1 time may be determined by whether TCP_CORK is supported...

@dentarg dentarg mentioned this pull request Mar 5, 2023
7 tasks
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

2 participants