Allow reading the command and status in the exception#71
Conversation
|
I noticed that I couldn't examine the command that threw the exception. So it's a quick fix for that. |
|
Nice, could you add some test for this? To in some moment some people not remove and break the dev scripts |
|
Thanks for the PR |
|
will do |
|
Full disclosure, I used AI to help with the spec. I'm not super proficient with specs yet. |
|
No problem, thanks for your honesty. I saw the tests here, but has some things that i like to do a few things that make me sure that the test is covering other possible problems as well. I will send you here ideas in some hours, and you check what you think |
|
Sounds good |
spec/error_spec.rb
Outdated
| require_relative "../lib/rubyshell/error" | ||
| require_relative "../lib/rubyshell" |
There was a problem hiding this comment.
Actually the Rspec are already eagerloading this files, so, is not being needed to require manually
spec/error_spec.rb
Outdated
| let(:command) { "ls -z" } | ||
| let(:stderr) { "ls: illegal option -- z" } | ||
| let(:status) { 1 } | ||
|
|
||
| subject(:error) do | ||
| described_class.new( | ||
| command: command, | ||
| stderr: stderr, | ||
| status: status | ||
| ) | ||
| end |
There was a problem hiding this comment.
Generally, i prefer to use a real system feature to run the test scenario, instead just stub a part of the code.
Instead create a error manually and pass his params, i used to do something like:
let(:error_instance) do
sh.ls(-z)
rescue error
error
end
describe 'Methods' do
describe 'attr_readers' do
it 'has command reader' do
expect(error_instance.command).to eq('ls -z')
end
it 'has status reader' do
expect(error_instance.status).to eq(1)
end
end
endThere was a problem hiding this comment.
-
In the expectations, i like to hardcode the expected value to be sure that the tests cannot be changed unintentionally in the future when there are more scenarios.
-
I like to create some "folder" separations between the tests soon as possible (Using the describe "Methods")
-
Like i said above, for me, is better to have a test that use the real code to setup the scenario, instead "mocking" the execution to go to a specific way
There was a problem hiding this comment.
I would like to respectfully disagree. Since these tests might run on a user's system and we don't know what he/she might have aliased the command to, I prefer not to invoke any real executables.
There was a problem hiding this comment.
Certainly any users machine has the ls command xD.
And the rest of the tests of the lib already use this for validation, so, this will not be a problem here. And if in some situation start to be a problem, we will need to fix in the entire application tests, or the user will run the test on a container (i will be able to configure a docker compose for this).
|
@gerases See the comments and let me know what you think |
|
I've refactored the test, let me know if you're ok with it |
|
Looks nice! Thanks! |
2 participants
No description provided.