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

Send one character (no newline) to stdout in protocol test to guarantee a single "message" and thus a single custom value #6978

Merged

Conversation

christian-monch
Copy link
Contributor

@christian-monch christian-monch commented Aug 24, 2022

Fixes issue #4921

This PR modifies the test datalad.support.tests.test_witless_runner.test_parameterized_protocol()
to only send a single character to stdout of the subprocess, i.e. 1. Previously two characters were
sent, i.e. 1 and \n. Depending on the behavior of the stdout-pipe, the data might be split up or
delivered in toto. In the first case, which is rare, the protocol.pipe_data_received() will be called
twice, leading to an unexpected result.

Changelog

🐛 Bug Fixes

This commit modifies
datalad.support.tests.test_witless_runner.test_parameterized_protocol()
to only send a single character to the subprocess. That should fix
issue datalad#4921, where protocol.pipe_data_received() might be triggered
twice, if the stdout-pipe decides to split up the previously used
two-characters string.

In addition the commit disabled the result renderer for add in
datalad.support.tests.test_annex_repo.test_files_split() to reduce
the number of logged output lines.
@codeclimate
Copy link

codeclimate bot commented Aug 24, 2022

Code Climate has analyzed commit bc4ded8 and detected 0 issues on this pull request.

View more on Code Climate.

@christian-monch christian-monch added the semver-tests Changes only affect tests, no impact on version label Aug 24, 2022
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • ideally should be against maint not master
  • moving (will remove there) my question/comment from the underlying issue:

Thanks @christian-monch for the analysis!!! I fail to see how/why changing the test "fixes" the situation in christian-monch@bc4ded8#diff-472642261afcc22bd6f543b4a8c20533588ec9281bd964137b29f79885e56164L197 ? Test code seems to be totally legit - just running a command which prints a single character, so should not be changed. IMHO it is the code of the runner is wrong somewhere that it manages to duplicate that single character, so better be fixed in the code (not in the test). Or what am I missing?

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #6978 (bc4ded8) into maint (29b42b4) will increase coverage by 0.54%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            maint    #6978      +/-   ##
==========================================
+ Coverage   90.13%   90.68%   +0.54%     
==========================================
  Files         354      354              
  Lines       46326    47355    +1029     
  Branches     6613     6609       -4     
==========================================
+ Hits        41755    42942    +1187     
+ Misses       4554     4397     -157     
+ Partials       17       16       -1     
Impacted Files Coverage Δ
datalad/runner/tests/test_witless_runner.py 95.39% <100.00%> (+2.40%) ⬆️
datalad/support/tests/test_annexrepo.py 97.97% <100.00%> (ø)
datalad/distribution/create_sibling.py 52.95% <0.00%> (-16.85%) ⬇️
datalad/local/remove.py 88.73% <0.00%> (-8.29%) ⬇️
datalad/support/tests/test_repo_save.py 95.34% <0.00%> (-4.66%) ⬇️
datalad/support/gitrepo.py 89.56% <0.00%> (-3.19%) ⬇️
datalad/tests/utils_pytest.py 87.05% <0.00%> (-3.08%) ⬇️
datalad/dataset/gitrepo.py 96.15% <0.00%> (-0.67%) ⬇️
datalad/runner/gitrunner.py 92.19% <0.00%> (-0.31%) ⬇️
datalad/_version.py 45.68% <0.00%> (-0.28%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@christian-monch
Copy link
Contributor Author

christian-monch commented Aug 24, 2022

  • ideally should be against maint not master
  • moving (will remove there) my question/comment from the underlying issue:

Thanks @christian-monch for the analysis!!! I fail to see how/why changing the test "fixes" the situation in christian-monch@bc4ded8#diff-472642261afcc22bd6f543b4a8c20533588ec9281bd964137b29f79885e56164L197 ? Test code seems to be totally legit - just running a command which prints a single character, so should not be changed. IMHO it is the code of the runner is wrong somewhere that it manages to duplicate that single character, so better be fixed in the code (not in the test). Or what am I missing?

The test code did NOT send a single character! It sent two characters. The reason is that the print-function appends an "\n" to the string that it writes out. Unless you specify the keyword-argument end to be something else, in this case we specify end="". Then we get only a single character printed.

❱ python -c "print('1')"|wc -c                                                                                                                                                                               130 !
2
❱ python -c "print('1', end='')"|wc -c
1

@christian-monch christian-monch changed the base branch from master to maint August 24, 2022 18:40
@christian-monch
Copy link
Contributor Author

christian-monch commented Aug 24, 2022

  • ideally should be against maint not master

Thx. Fixed!
I actually branched of maint. Always forget to change the merge-target in the PRs ;-)

@yarikoptic
Copy link
Member

The test code did NOT send a single character! It sent two characters

you kept saying that and I was "why he keeps saying that?" ;) Looked at the test code and the purpose of that test and got it - indeed there is an assumption in the test that there would be a single message/invocation of the pipe_data_received where we replace with our custom (5) value. And that made me understand @christian-monch ! ;-) Thanks! Let's proceed - I can't resist such a green CI ;-)

@christian-monch christian-monch changed the title send one character to stdin in protocol test send one character to stdout in protocol test Aug 24, 2022
@yarikoptic yarikoptic changed the title send one character to stdout in protocol test Send one character (no newline) to stdout in protocol test to guarantee a single "message" and thus a single custom value Aug 24, 2022
@yarikoptic yarikoptic merged commit 3300769 into datalad:maint Aug 24, 2022
@christian-monch christian-monch deleted the issue-4921-output-duplication branch August 24, 2022 19:54
@github-actions
Copy link

🚀 PR was released in 0.17.4 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released semver-tests Changes only affect tests, no impact on version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.13.3 on conda and fresh git-annex: duplicated output picked up within test_runner_parametrized_protocol?
2 participants