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

improve CLI tests #1710

Merged
merged 1 commit into from
Apr 14, 2023
Merged

improve CLI tests #1710

merged 1 commit into from
Apr 14, 2023

Conversation

pmeier
Copy link
Member

@pmeier pmeier commented Apr 12, 2023

What does this implement/fix?

Our current CLI tests are not sufficient, since our checking logic does not do what it is supposed to be doing. We only catch errors in the subprocess:

nebari/tests/test_cli.py

Lines 15 to 23 in daecbcf

def run_cli_cmd(command: str, working_dir: typing.Union[str, Path]):
"""Run the provided CLI command using subprocess."""
try:
os.chdir(working_dir)
subprocess.call(command.split())
except subprocess.CalledProcessError:
return False
return True

However, subprocess.call does not fail on exit codes > 0:

$ ls foo
"foo": No such file or directory (os error 2)
$ echo $?
2
$ python -c 'import subprocess; subprocess.call(["ls", "foo"])'
ls: cannot access 'foo': No such file or directory
$ echo $?
0

This PR uses subprocess.run instead, which is the recommended way and has the check=True flag to enable error code checking.

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

tests/test_cli.py Show resolved Hide resolved
tests/test_cli.py Show resolved Hide resolved
@pmeier pmeier mentioned this pull request Apr 12, 2023
10 tasks
@pavithraes pavithraes added type: enhancement 💅🏼 New feature or request area: testing ✅ Testing needs: review 👀 This PR is complete and ready for reviewing area: nebari-cli labels Apr 12, 2023
@pavithraes pavithraes requested a review from iameskild April 12, 2023 13:32
Copy link
Member

@iameskild iameskild left a comment

Choose a reason for hiding this comment

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

Nice catch @pmeier! This looks good to me!

@pmeier pmeier merged commit c72937d into nebari-dev:develop Apr 14, 2023
@pmeier pmeier deleted the improve-cli-test branch April 14, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: nebari-cli area: testing ✅ Testing needs: review 👀 This PR is complete and ready for reviewing type: enhancement 💅🏼 New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

3 participants