Tests for git / github module - adventures in patching and mocking #445
Conversation
lwasser
changed the title
Codecov Report
@@ Coverage Diff @@
## main #445 +/- ##
===========================================
+ Coverage 50.44% 60.79% +10.34%
===========================================
Files 22 22
Lines 1447 1584 +137
===========================================
+ Hits 730 963 +233
+ Misses 717 621 -96
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| elif org == "earthlab" and repository == "test-student": | ||
| pass | ||
| else: | ||
| raise Exception |
There was a problem hiding this comment.
So there is a specific exception raised. we prbably want to capture that but need to decide on what package first.
|
ALSO: why is code cov looking at coverage for a test file? |
| # @staticmethod | ||
| def create_repository(self, repository): | ||
| if repository == "test_repo": | ||
| return True |
There was a problem hiding this comment.
Why is code cov running test coverage on mock objects? do we have to decorate these in some way?
|
|
||
| @pytest.fixture() | ||
| def example_student_repo(): | ||
| """A fixture with an example student repo.""" |
There was a problem hiding this comment.
this is the start of a fake repo fixture that i can make - here i would init the repo and drop it wherever the test needed it and then we could run checks on it.
|
|
||
| @mock.patch("subprocess.run") | ||
| def test_clone_repo_pass2( | ||
| mock_subproc_run, monkeypatch, example_student_repo, capsys |
There was a problem hiding this comment.
here i'm using pytest monkeypatch AND mock_subproc_Run from unittest
is that reasonable? i like mock but monkeypatch is easier for me. but mock seems to better handle subprocess stuff
lwasser
changed the title
| @@ -25,7 +25,13 @@ def get_github_auth(): | |||
| """ | |||
| yaml = YAML() | |||
| try: | |||
| with open(op.expanduser("~/.abc-classroom.tokens.yml")) as f: | |||
| with open( | |||
| op.join(op.expanduser("~"), ".abc-classroom.tokens.yml") | |||
There was a problem hiding this comment.
QUESTION: why is this function in config vs github? it doesn't make sense to me here. but i could be convinced otherwise.
Also a few places conflate expand user with creating a path. this makes testing harder because we want to mock out the specific users home directory. so this needs to be this way to ensure we can easily mock.
Just make sure i didn't break other things by doing this.
There was a problem hiding this comment.
@kcranston this is a question about module organization. i'm just curious why some of the github helpers are in config. it's worth just considering organization and i'm very open to whatever we decide i just didn't expect to look for the token helper in config given all other functions are in github. @nkorinek open to your thoughts too!!
There was a problem hiding this comment.
Only because config.py is where the authorization functions were when I started on the project. I am not opposed to moving all of the git and github stuff into the same place!
There was a problem hiding this comment.
hi!! ok wonderful. it may make more sense. i started writing tests and realized i had to write tests for config in the github file atleast initially... so perhaps moving it will be good.
There was a problem hiding this comment.
ok @kcranston you should be able to run the code below. Notice that no matter what i do i can't get to the FileNotFoundError. This is because if there's no standard error it fails at the startswith check because there is no error message. And otherwise it goes to RunTimeError i'm just confused how to hit that last conditional and haven't been able to test on a computer without SSH setup yet.
import subprocess
import abcclassroom.github as github
import unittest.mock as mock
cmd = ["ssh", "-T", "git@github.com"]
# When i try to mock with a side effect of FileNotFound it doesn't seem
# to mock correctly. rather it just runs and says no error so the mock
# isn't working as i think it should
try:
with mock.patch('subprocess.run') as mock_requests:
mock_requests.side_effect = FileNotFoundError
github.check_git_ssh()
print("no error")
except subprocess.CalledProcessError as e:
print("called process error")
except FileNotFoundError as e:
print("File error")
# This gets us to if subprocess_out.startswith("Hi"): via successful authentication
# i just confused myself because why isn't it raises a subprocess error.
# but it is getting to the correct line of code in the function "hi username"
try:
with mock.patch('subprocess.run') as mock_requests:
mock_requests.side_effect = subprocess.CalledProcessError(
returncode=128, cmd=cmd, stderr="Hi")
github.check_git_ssh()
print("no error")
except subprocess.CalledProcessError as e:
print("called process error")
except FileNotFoundError as e:
print("File error")
# This raises a called process error as we'd expect the function to do
try:
with mock.patch('subprocess.run') as mock_requests:
mock_requests.side_effect = subprocess.CalledProcessError(
returncode=128, cmd=cmd, stderr="Warning: Permanently")
github.check_git_ssh()
print("no error")
except subprocess.CalledProcessError as e:
print("called process error")
except FileNotFoundError as e:
print("File error")
# This gets us to a runtime error as we would expect it to.
try:
with mock.patch('subprocess.run') as mock_requests:
mock_requests.side_effect = subprocess.CalledProcessError(
returncode=128, cmd="", stderr="Encountered this error ")
github.check_git_ssh()
print("no error")
except RuntimeError as e:
print("Runtime error raised")
except subprocess.CalledProcessError as e:
print("called process error")
except FileNotFoundError as e:
print("File error")
| @@ -61,6 +59,9 @@ def get_access_token(): | |||
| return access_token | |||
|
|
|||
|
|
|||
| # TODO: document this function with parameters and returns but also | |||
| # should it provide a user friendly message based upon where it fails or | |||
| # doesnt fail? | |||
| def check_git_ssh(): | |||
| """Tests that ssh access to GitHub is set up correctly on the users | |||
| computer. | |||
There was a problem hiding this comment.
hey there @kcranston me again. i think i'm getting a better handle on how this works now but i have a question about this function. The comment below says " We ALWAYS get here". This is true - it seems like when you use check=True it it returns an error w a non zero exit code.
My question is - why do we use check=True here? Without it, it just seems to run and return:
Out[6]: CompletedProcess(args=['ssh', '-T', 'git@github.com'], returncode=1, stdout='', stderr="Hi lwasser! You've successfully authenticated, but GitHub does not provide shell access.\n")
is this potentially because i may see different behavior when running at the command line? just trying to understand as i'm writing tests. it's more complex to route through all of these try/except blocks and more complex to test this way but perhaps there is a reason that i just don't understand to force that error to be thrown when using subprocess w/ check=True
There was a problem hiding this comment.
This particular case is strange, because the subprocess call always returns a non-zero exit code. So, we need to check the specific error message from git to see what's going wrong. If we re-write with check=False, then I think the _call_git function won't return the git messages (because it won't catch a CalledProcessError).
There was a problem hiding this comment.
ahhh so it's because without raising some sort of error, we can't parse what happened.
Oh also i just thought of this - because it's a subprocess call would it also just fail or pass quietly without check=True ? is that essentially what you're saying? i think that makes sense to me if it's running at the command line, how could it catch a failure.
i just tried it with a fake git command and i see it "passed" but failed at the CLI
There was a problem hiding this comment.
Yup - you are correct (in both assumptions). We don't want it to fail quietly, and we need to get the git output to parse exactly what goes wrong. (The reason this always fails is that ssh -T git@github.com always returns a non-zero exit code, because github does not actually allow shell access, even if ssh is set up correctly).
There was a problem hiding this comment.
ok i understand @kcranston i'll work on testing through all of the try/excep pieces!! this is super helpful!
| # a non-zero exit code whether ssh access is set up correctly or | ||
| # not. Must check output. | ||
| # not. We then parse output to see what was returned. |
There was a problem hiding this comment.
hey @kcranston i need a bit of advice here. i'm having a hard time triggering a filenotfounderror for testing. it seems to always go to that final else statement when i trigger any type of error. Just wondering how you managed to get to that part of this conditional? any advice is welcome. i will comment out the test that i have for now and move on but am hopeful you have some ideas!
There was a problem hiding this comment.
Oh gosh. It's possible that there is a bug here. I recall going through a lot of contortions to get my machine not set up correctly in order to trigger the various failures. Testing this requires uninstallation of open-ssh.
There was a problem hiding this comment.
ahhhh ok i think i understand. I think i also found a simpler version of the mock test code that is reproducible that i can share. i will try to pull that together so you can kind of just run it to see what works and doesn't work. i've managed to test the entire function with the exception of that last part. we have to mock it because so many users will already have ssh setup. (i think) but it should work properly on CI because ssh won't be setup there i think
@nkorinek i'm just playing with some of the tests. i've added some notes and todos. we can talk about this more next week but some of the tests weren't running correctly for me locally such as git branc for
_call_git. so i'm working through them. but generally i think what you setup is great and will work for us. we just need to think through exactly what we are testing.NOTES:
on branch... but all sub process testing will be a bit tricky because of this.I also noticed in many places we aren't capturing specific exceptions so we may want to talk about that more next week.
UPDATE
I am not convinced that
pytest.subprocessis doing what we want. it seems to work well when the subprocess is directly called but it was actually cloning on my computer. i found a great SO post that i linked to https://stackoverflow.com/questions/25692440/mocking-a-subprocess-call-in-python that usesunittest.mock. i was able to mock things successfully (i think) but need to look at it again with fresh eyes. there is one part i'm not sure about. i then captured the expected return standard out feedback from clone and tested against that. Do we also want to create a fixture (i started this) with a fake git repo and test that the repo was placed in the correct spot? it seems a bit contrived but i did start to create that fixture. We'd then want to use tmp_path fixture to avoid saving any files to a users directory.Adding this resource
pytest-dev/pytest#4576
For a discussion on mock vs monkeypatch in unittest vs pytest.