Skip to content

Fix test_replace_pos_args_empty_sys_argv #1936

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

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

hexagonrecursion
Copy link
Contributor

test_replace_pos_args_empty_sys_argv was identical to test_replace_pos_args_none_sys_argv I assume this is not intentional. This PR changes test_replace_pos_args_empty_sys_argv to test pos_args=[] instead of pos_args=None

Contribution checklist:

(also see CONTRIBUTING.rst for details)

  • wrote descriptive pull request text
  • added/updated test(s)
  • updated/extended the documentation - N/A
  • added relevant issue keyword - N/A
    in message body
  • added news fragment in changelog folder - too minor
    • fragment name: <issue number>.<type>.rst for example (588.bugfix.rst)
    • <type> is must be one of bugfix, feature, deprecation,breaking, doc, misc
    • if PR has no issue: consider creating one first or change it to the PR number after creating the PR
    • "sign" fragment with "by :user:<your username>"
    • please use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files - by :user:superuser."
    • also see examples
  • added yourself to CONTRIBUTORS (preserving alphabetical order) - CONTRIBUTORS: No such file or directory

This test was identical to test_replace_pos_args_none_sys_argv. I assume this is not intentional
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #1936 (3259ef9) into rewrite (b88679a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           rewrite    #1936   +/-   ##
========================================
  Coverage    99.82%   99.82%           
========================================
  Files          143      143           
  Lines         7839     7839           
  Branches       789      789           
========================================
  Hits          7825     7825           
  Misses           3        3           
  Partials        11       11           
Flag Coverage Δ
tests 99.82% <100.00%> (ø)

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

Impacted Files Coverage Δ
.../config/loader/ini/replace/test_replace_posargs.py 100.00% <100.00%> (ø)

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 b88679a...3259ef9. Read the comment docs.

@gaborbernat
Copy link
Member

Why? 🤔

@hexagonrecursion
Copy link
Contributor Author

Maybe I'm missing something, but the tests look exactly the same to me. Current rewrite:

def test_replace_pos_args_none_sys_argv(replace_one: ReplaceOne) -> None:
result = replace_one("{posargs}", None)
assert result == ""
def test_replace_pos_args_empty_sys_argv(replace_one: ReplaceOne) -> None:
result = replace_one("{posargs}", None)
assert result == ""

I just assumed that one of them was meant to test None and the other []

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat merged commit 651e1ae into tox-dev:rewrite Mar 1, 2021
@hexagonrecursion hexagonrecursion deleted the patch-1 branch March 1, 2021 17:36
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.

2 participants