Build tidiness#233
Merged
Merged
Conversation
* Use Timeout.timeout, not Object#timeout
* http://git.io/vJf3J * This appears consistently to fix the red Reel tests
Contributor
Author
|
Oh my word, github, you really are in a mood tonight. |
* Avoids NoMethodError: undefined method `spec' for nil:NilClass * Similar approach used in thoughtbot/factory_bot@3c0547f
rgarner
force-pushed
the
build-tidiness
branch
2 times, most recently
from
cab9deb to
0899628
Compare
In order to 'reserve' a free TCP Server port, we create a TCPServer, take a note of its port number, and then immediately close it. We'd then ask our adapter to use the port we just finished with via that adapter's configuration. While this isn't a particularly safe way of 'reserving' a port, it's the best we can do with heterogeneous adapters that have a one-way config method. However, this would cause intermittent test failures when our 'canary' socket did not get out of the way in time for an adapter to bind its server. Socket#close does not actually block until the socket is closed at OS level, it just frees the Ruby handle to it. Actually waiting for the socket to become free or garbage-collecting it to force the issue is counterproductive – we'd need to poll the port or run a full GC, which would take more time than a simple sleep. Additionally, the sleep needs to be tuned so that tests are not unduly slowed by it – too quick and we'll see failures, too slow and our builds will reflect it.
Using let-style RSpec and lazily creating our servers before every test is good from a test purity point of view but increases greatly the risk of port collisions when we attempt to find a free port to run our adapter in adapter_lint (bearing in mind that Socket#close only releases the Ruby resources, and the socket persists in a TIME_WAIT state for an indeterminate period of time at OS level). Revert to before(:all) and create only one server that will be used for the duration of all tests in adapter_lint. We still need to sleep to ensure our 'free' ports are available, but we reduce the risk of servers being unavailable. This also has the happy side effect of making the tests somewhat faster.
Member
|
This looks good to me. Is there a reason not to have Ruby 2.2 in the build? |
Contributor
Author
|
No good reason, no. Really I was just avoiding it to try and make the build time more tolerable while still acknowledging latest stable. |
For reasons we can't currently fathom, Rubinius has a tendency to fail the build. Rather than let this continue to be the case, until such time as someone has time to figure out precisely why, unblock other PRs by letting builds pass with MRI Rubies for now.
Contributor
|
Should we wait for this to be merged into master before merging into other PRs (like #235), or would it be fine to pull in to them now to get tests passing? |
Member
|
I am going to run this a couple times on travis now. |
Member
|
builds all the time, merge merge. |
Member
|
Next step would be to extract all adapters except those shipped by ruby itself to be moved to different gems. Could there be a way to load them automatically when needed? @seancribbs @rgarner |
Contributor
Author
|
The relief ... now to work out why Rubinius is such a pain and fix it. |
Closed
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Object#timeoutdeprecation warnings when building with 2.3.0Socket#closedoes not actually block on closing the socket. Ruby thinks it's closed, but the OS has it in a briefTIME_WAITstate. There's seemingly no good way of detecting this, so we combinesleeping after close with reverting tobefore(:all)for adapter lint tests and having each adapter's server live for the duration of alladapter_linttests instead of restarting on every test.NoMethodError: undefined methodspec' for nil:NilClass` which occurred in various of Travis's rubies due to old bundler versions by updating bundler before build