Update ruby versions #208
Update ruby versions #208
Conversation
| @@ -2,6 +2,8 @@ rvm: | |||
| - 2.1.9 | |||
| - 2.2.4 | |||
There was a problem hiding this comment.
It may make sense to remove these EOL versions too?
There was a problem hiding this comment.
Yes, I think it does. I just wasn't sure if we wanted to leave those in for now, and was planning to make another PR to take them out once we'd established that the newer versions of ruby worked.
I need to make an update to the README as well, once we know these are working.
There was a problem hiding this comment.
readme is updated.
Do you think we should remove the old versions of ruby now, or leave them in place for a bit in case there are any bugs we're not aware of?
There was a problem hiding this comment.
Excellent!
No idea about removing the EOL versions. Maybe a project maintainer will give good advice in respect to this 😉
.travis.yml
Outdated
| @@ -2,6 +2,8 @@ rvm: | |||
| - 2.1.9 | |||
| - 2.2.4 | |||
| - 2.3.1 | |||
| - 2.4.2 | |||
| - 2.5.1 | |||
There was a problem hiding this comment.
Not sure about this: if we use major.minor (e.g. 2.5), won't we test against the latest release of Ruby as soon as a newer 2.5.x version is released?
There was a problem hiding this comment.
that would be based on the rvm version that's installed... I'm not very familiar with travisci's config, and was just following the pattern of the previous entries.
There was a problem hiding this comment.
I'm not a travis expert neither. Looking at other projects which use X.Y, I see these versions in a recent build:
.travis.yml |
Ruby version used | Latest ruby version available |
|---|---|---|
| 2.3 | 2.3.4 | 2.3.7 |
| 2.4 | 2.4.1 | 2.4.4 |
| 2.5 | 2.5.1 | 2.5.1 |
This look… inconsistent 😄
There was a problem hiding this comment.
agreed. I just noticed the same in the travis builds. seems like I should leave the patch versions in place so we know what we're getting?
lib/spinach/reporter/reporting.rb
Outdated
| @@ -126,7 +126,7 @@ def summarized_error(error) | |||
| # The coplete error report | |||
| # | |||
| def full_error(error) | |||
| feature, scenario, step, exception = error | |||
| _, _, step, exception = error | |||
There was a problem hiding this comment.
_feature and _scenario are probably better names for unused variables?
There was a problem hiding this comment.
they were throwing wanings due to being assigned and never used. They aren't actually part of the test, which is why I removed them. Makes me wonder if maybe the error method should be returning something different? Or there's something else we should be testing that we aren't?
I'm ok with leaving them in, or changing the tests.
There was a problem hiding this comment.
You may be referring to rubocop's Lint/UselessAssignment… If so, prepending a _ at the beginning of the variable name silents this warning. Here, we call a method which return an array of 4 elements, and we are only interested in the 3rd and 4th ones, so yes, we need some "placeholders" to store the 1st and 2nd.
Using just _ as a name is possible but is quite obscure. For the sake of readability, it may make sense to keep the original names.
This is clearly just a matter of style 😉
There was a problem hiding this comment.
agreed that it's not as clear. I'll back this out.
|
@oriolgual @josepjaume could you review please? Also need to know if you'd like me to remove the ruby 2.1 and 2.2 versions from travis.yml. Current travis failure is caused by jruby, which is fixed in #207. |
|
Fixed by #243 |
ruby 2.1 and 2.2 are both EOL for support, and ruby 2.3 EOL is coming up next March.
This PR cleans up some of the warnings in the test output, uses ruby 2.3.7 and adds ruby 2.4.4 and 2.5.1 to the travis file.