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

NullIO#closed? should return false #2883

Merged
merged 1 commit into from Jun 2, 2022
Merged

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented May 20, 2022

Description

Currently returns true.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@MSP-Greg MSP-Greg changed the title NullIO#closed should return false NullIO#closed? should return false May 20, 2022
@MSP-Greg MSP-Greg changed the title NullIO#closed? should return false NullIO#closed? should return false, TruffleRuby skip May 20, 2022
@MSP-Greg
Copy link
Member Author

@eregon Sorry for the ping. The test mentioned above fails frequently on truffleruby. Every time I've seen the error, I check truffleruby-head, and it passes there. The test is here. If you have any ideas...

JFYI, Puma is testing on Windows mswin...

@MSP-Greg MSP-Greg added the bug label May 20, 2022
@eregon
Copy link
Contributor

eregon commented May 26, 2022

I have no specific idea, I think adding the skip is fine here if needed, although a check against RUBY_ENGINE_VERSION would be better since it seems already fixed on truffleruby-head (and so should be for the next release as well).

The sleep 0.1 in that test are probably very timing-sensitive though (and make the test slow), that should probably be avoided if possible.

@MSP-Greg
Copy link
Member Author

a check against RUBY_ENGINE_VERSION

I'm a bit TruffleRuby challenged. Would RUBY_ENGINE_VERSION < '22.2' be okay? Or, something a bit more complicated?

The sleep 0.1 in that test are probably very timing-sensitive though (and make the test slow

Given how long a few other tests take, not concerned about time. Also, some of the timing is related to Actions (one VM on a four VM system). Or, many of the test delays aren't needed when I run locally...

@eregon
Copy link
Contributor

eregon commented May 27, 2022

omit/skip unless Gem::Version.new(RUBY_ENGINE_VERSION) > Gem::Version.new('22.1') seems more robust and is true for both 22.2.0-dev-8839057f and upcoming 22.2.

@MSP-Greg
Copy link
Member Author

@eregon - Thanks, updated.

@MSP-Greg MSP-Greg changed the title NullIO#closed? should return false, TruffleRuby skip NullIO#closed? should return false May 30, 2022
@nateberkopec nateberkopec merged commit 25740ea into puma:master Jun 2, 2022
@MSP-Greg MSP-Greg deleted the 00-nullio branch June 2, 2022 20:49
nateberkopec pushed a commit that referenced this pull request Aug 22, 2022
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants