-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
@@ -4,11 +4,14 @@ cache: bundler | |||
rvm: | |||
- 2.1.8 | |||
- 2.2.4 | |||
- 2.3.0 | |||
- 2.3.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much more used Ruby version I think. And it's what we use primarily right now.
@@ -329,7 +329,7 @@ class Measured::Rails::ActiveRecordTest < ActiveSupport::TestCase | |||
end | |||
|
|||
test "assigning a number with more significant digits than permitted by the column precision does not raise exception when it can be rounded to have lesser significant digits per column's scale" do | |||
assert_nothing_raised Measured::Rails::Error do | |||
assert_nothing_raised do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a legit failure. There was two definitions of this method before, one took (*args)
and ignored them. This is the correct syntax now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove assert_nothing_raised
now, and let unexpected exceptions bubble up to the test runner?
All the same comments as Shopify/active_shipping#424 for this one |
b9cb09e
to
a800ed8
Compare
Removed branch reference. I'm not too worried about allowed failures. We should fix the things that break as they break. |
matrix: | ||
exclude: | ||
- gemfile: Gemfile | ||
rvm: 2.1.8 | ||
- gemfile: gemfiles/rails-master.gemfile | ||
rvm: 2.1.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we're not excluding gemfiles/rails-4.2.gemfile
? If we were, we could just drop 2.1.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we want to support lower versions of ruby. But we have to exclude it from rails 5+ as rails 5 needs ruby 2.2.2 or higher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so we're still testing our code with 2.1.8, just using another gemfile. I wasn't 100% sure how it all worked but I grok it now. Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! It takes the matrix of all gemfiles and all ruby versions, except for these combinations that we manually exclude because they are invalid.
@@ -329,7 +329,7 @@ class Measured::Rails::ActiveRecordTest < ActiveSupport::TestCase | |||
end | |||
|
|||
test "assigning a number with more significant digits than permitted by the column precision does not raise exception when it can be rounded to have lesser significant digits per column's scale" do | |||
assert_nothing_raised Measured::Rails::Error do | |||
assert_nothing_raised do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove assert_nothing_raised
now, and let unexpected exceptions bubble up to the test runner?
I have no strong feelings about |
@benwah @thegedge @sgrif
https://twitter.com/sgrif/status/804037955241312258
Run against rails master in CI.