Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

verdi computer test: fix bug in spurious output test #4316

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Aug 20, 2020

Fixes #4314

The test that checks for spurious output when executing a normal command
on a computer over the transport had a bug in it that went unnoticed
because the code path was not tested. If the stderr of the command
contained any output the command would raise because the test that is
called _computer_test_no_unexpected_output would incorrectly return a
tuple of length one instead of two in that case.

In addition to adding tests to hit this code path, the message that is
printed in the case of non-empty stdout or stderr is deduplicated and
adapted to be bit clearer and refer directly to the documentation
instead of through a Github issue.

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #4316 into develop will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4316      +/-   ##
===========================================
+ Coverage    79.04%   79.10%   +0.07%     
===========================================
  Files          468      468              
  Lines        34610    34611       +1     
===========================================
+ Hits         27354    27376      +22     
+ Misses        7256     7235      -21     
Flag Coverage Δ
#django 72.72% <100.00%> (+0.06%) ⬆️
#sqlalchemy 71.91% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/cmdline/commands/cmd_computer.py 80.88% <100.00%> (+1.98%) ⬆️
aiida/transports/plugins/local.py 81.29% <0.00%> (+0.26%) ⬆️
aiida/transports/transport.py 63.68% <0.00%> (+0.45%) ⬆️
aiida/engine/daemon/client.py 73.57% <0.00%> (+1.15%) ⬆️
aiida/schedulers/plugins/direct.py 59.07% <0.00%> (+5.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50988f5...c85e929. Read the comment docs.

@ramirezfranciscof
Copy link
Member

So, this is just 100% hunch, but where you are defining the exec_command_wait in each test, shouldn't one of those have the stdout/stderr and the empty string be switched?

@sphuber
Copy link
Contributor Author

sphuber commented Aug 21, 2020

So, this is just 100% hunch, but where you are defining the exec_command_wait in each test, shouldn't one of those have the stdout/stderr and the empty string be switched?

It absolutely should, just forgot to switch them. Will update it, thanks

@sphuber sphuber force-pushed the fix/4314/verdi-computer-test-spurious-stderr branch from e7d1689 to e1d9ecb Compare August 21, 2020 15:56
@giovannipizzi
Copy link
Member

Thanks!
I'd be ready to approve, but maybe in the template can you please add another {} and say if the string originates from the stdout or the stderr?
Something like We detected some spurious output in {} ... and you set that to the string stderr or stdout accordingly

The test that checks for spurious output when executing a normal command
on a computer over the transport had a bug in it that went unnoticed
because the code path was not tested. If the `stderr` of the command
contained any output the command would raise because the test that is
called `_computer_test_no_unexpected_output` would incorrectly return a
tuple of length one instead of two in that case.

In addition to adding tests to hit this code path, the message that is
printed in the case of non-empty stdout or stderr is deduplicated and
adapted to be bit clearer and refer directly to the documentation
instead of through a Github issue.
@sphuber sphuber force-pushed the fix/4314/verdi-computer-test-spurious-stderr branch from e1d9ecb to c85e929 Compare August 26, 2020 20:18
@sphuber
Copy link
Contributor Author

sphuber commented Aug 26, 2020

@giovannipizzi done!

@giovannipizzi giovannipizzi merged commit bc52ab1 into develop Aug 27, 2020
@giovannipizzi giovannipizzi deleted the fix/4314/verdi-computer-test-spurious-stderr branch August 27, 2020 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception in verdi computer test if spurious output in stderr
3 participants