Litmus conversion #1175
Litmus conversion #1175
Conversation
pmcmaw
force-pushed
the
litmus_conversion
branch
13 times, most recently
from
e995ca7
to
95df01e
Compare
pmcmaw
force-pushed
the
litmus_conversion
branch
7 times, most recently
from
2599ae4
to
b995f1b
Compare
pmcmaw
force-pushed
the
litmus_conversion
branch
6 times, most recently
from
3a30dcb
to
c4c0257
Compare
tphoney
force-pushed
the
litmus_conversion
branch
from
0fa404b
to
941e90c
Compare
| end | ||
| end | ||
|
|
||
| describe 'mysqlbackup.sh' do | ||
| describe 'mysqlbackup.sh', if: Gem::Version.new(mysql_version) < Gem::Version.new('5.7.0') do |
There was a problem hiding this comment.
Instead of putting the skip at this level, you might consider putting a skip in a before block using this condition as in this change.
The advantage gained is that you can then see in the CI output exactly which it blocks were skipped, which gives increased visibility into exactly how many tests are not being run on a give CI configuration and for what reason, while still reducing the verbosity and repetitiveness of the previous pattern of the unless block in every test.
We moved to this pattern recently because our inability to answer that question was causing issues for us.
| shell('/usr/local/sbin/mysqlbackup.sh') do |r| | ||
| expect(r.stderr).to eq('') | ||
| end | ||
| run_shell('/usr/local/sbin/mysqlbackup.sh') do |r| |
There was a problem hiding this comment.
Does the module create the file mysqlbackup.sh? It seems like this command run_shell('/usr/local/sbin/mysqlbackup.sh') could be moved into a before(:each) block, and then the it's on 39 and 52 could be removed in favor of simply the two it's on 45 and 58.
This would have the additional advantage of more properly modeling the relationship that needs to occur between the run_shell command that needs to run before each test, and the test itself. Currently the run_shell is in an it that is not specifically related to the it immediately preceding. The it on 39 for example seems like it's just test setup for the it it on 45, but that relationship is not currently modeled by the way the file is arranged.
I have no expectation that the current arrangement would fail any time soon, so I'm not proposing to hold up merge for this suggestion.
|
@RandomNoun7 whole heartedly agree with both of these, i can clean this up after we get this merged. I had to go through a horror show of a rebase 🤢 due to the multiple commits. |
No description provided.