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.

Sign up for GitHub

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

Tests for git / github module - adventures in patching and mocking #445

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from
Draft

Tests for git / github module - adventures in patching and mocking #445

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
bdef8ab
Reopening WIP pr
nkorinek Apr 8, 2020
db1a2d7
flake8
Oct 22, 2021
5cbd918
add subprocess to code cov build
Oct 22, 2021
5079c13
black
Oct 22, 2021
ce927ae
flake8
Oct 26, 2021
a14d126
Merge branch 'main' into nkorinek-monkey-patch
Oct 26, 2021
1bf8785
black
Oct 27, 2021
5b731f9
merge
Oct 27, 2021
9b4379f
flake8
Oct 27, 2021
d7015ed
clean up returnmessage
Oct 27, 2021
8f26cf0
remove unused imports
Oct 29, 2021
62185f5
flake8
Oct 30, 2021
634b085
black
Nov 1, 2021
4f36bd2
black
Nov 1, 2021
d0fb52e
black:
Nov 1, 2021
c40291b
flake8
Nov 2, 2021
5d2a6fe
black
Nov 9, 2021
403f7d6
last test for check ssh
Nov 9, 2021
884943a
flake8
Nov 11, 2021
cc25e70
update black
Nov 11, 2021
c4779a2
black
Nov 11, 2021
140176c
black
Nov 12, 2021
6b5e4b5
flake8
Nov 12, 2021
d994013
black
Nov 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/code-cov.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ jobs:
run: |
pip install pytest
pip install pytest-cov
pip install pytest-subprocess
lwasser marked this conversation as resolved.
Show resolved Hide resolved
pip install -e .
pytest --cov=./ --cov-report=xml
- name: Upload coverage to Codecov
Expand Down
8 changes: 7 additions & 1 deletion abcclassroom/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@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!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

@lwasser lwasser Nov 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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", "[email protected]"]
# 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")

) as f:
print(
"patched",
op.join(op.expanduser("~"), ".abc-classroom.tokens.yml"),
)
config = yaml.load(f)
return config["github"]

Expand Down
248 changes: 149 additions & 99 deletions abcclassroom/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
====================
"""

import os
import logging
# import os
# import logging
import random
import string
import subprocess
Expand Down Expand Up @@ -61,6 +61,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.
Copy link
Author

Choose a reason for hiding this comment

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

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', '[email protected]'], 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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 [email protected] always returns a non-zero exit code, because github does not actually allow shell access, even if ssh is set up correctly).

Copy link
Author

Choose a reason for hiding this comment

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

ok i understand @kcranston i'll work on testing through all of the try/excep pieces!! this is super helpful!

Expand Down Expand Up @@ -223,32 +226,73 @@ def _call_git(*args, directory=None):


def remote_repo_exists(org, repository, token=None):
"""Check if the remote repository exists for the organization."""
"""Check if the remote repository exists for the organization.
Parameters
----------
org : string
Name of the organization where the repo lives on GitHub.
repository : string
Name of the repository within the organization to clone.
token : string (default None)
Token value required for authentication

Returns
-------
Boolean True if exists, False / raises exception if it doesn't exist.

"""

try:
g = gh3.login(token=token)
g.repository(org, repository)

# TODO: this raises github3.exceptions.NotFoundError: 404 Not Found
# should capture the specific exception and then return false
except Exception:
return False

return True


# TODO: do neither github python package wrap clone for us?
def clone_repo(organization, repo, dest_dir):
"""Clone `repository` from `org` into a sub-directory in `directory`.
"""Clone `repository` from `org` into a sub-directory in `directory`
using ssh.

Raises RuntimeError if ssh keys not set up correctly, or if git clone
fails for other reasons.

Parameters
----------
organization : string
A string with the name of the organization to clone from
repo : string
A string with the name of the GitHub repository to clone
dest_dir : string
Path to the destination directory
TODO: is this a full path, path object or string - what format is
dest_dir in

Returns
-------
Cloned github repository in the destination directory specified.
"""

try:
# first, check that local git set up with ssh keys for github
check_git_ssh()
url = "[email protected]:{}/{}.git".format(organization, repo)
print("cloning:", url)

_call_git("-C", dest_dir, "clone", url)
print("Successfully cloned:", url)
# TODO: ? should we check where the error is - so is it an ssh error
# vs a can't find repo / bad url error here??
except RuntimeError as e:
print(
r"Oops, something went wrong when cloning {}\{}: \n".format(
organization, repo
)
)
raise e


Expand Down Expand Up @@ -411,102 +455,108 @@ def git_init(directory, defaultbranch="main"):
_call_git("init", directory=directory)


# TODO: let's remove these if we aren't using them?
# FOR NOW illl comment out but if we can decide to finally delete (i'm ok
# with this now) lets do it.
###################################################
# Methods below are from before the re-factoring.
# Retaining for reference, but with no guarantee
# about correct function.


def check_student_repo_exists(org, course, student, token=None):
"""Check if the student has a repository for the course.

It happens that students delete their repository or do not accept the
invitation to the course. In either case they will not have a repository
yet.
"""
# temporarily change log level of github3.py as it prints weird messages
# XXX could be done more nicely with a context manager maybe
gh3_log = logging.getLogger("github3")
old_level = gh3_log.level
gh3_log.setLevel("ERROR")

try:
g = gh3.login(token=token)
repository = "{}-{}".format(course, student)
g.repository(org, repository)

except Exception as e:
raise e

finally:
gh3_log.setLevel(old_level)


def close_existing_pullrequests(
org, repository, branch_base="new-material-", token=None
):
"""Close all oustanding course material update Pull Requests

If there are any PRs open in a student's repository that originate from
a branch starting with `branch_base` as name and created by the user
we are logged in we close them.
"""
g = gh3.login(token=token)
me = g.me()
repo = g.repository(org, repository)
for pr in repo.pull_requests(state="open"):
origin = pr.head.label
origin_repo, origin_branch = origin.split(":")
if origin_branch.startswith(branch_base) and pr.user == me:
pr.create_comment(
"Closed in favor of a new Pull Request to "
"bring you up-to-date."
)
pr.close()


def create_pr(org, repository, branch, message, token):
"""Create a Pull Request with changes from branch"""
msg_parts = message.split("\n\n")
if len(msg_parts) == 1:
title = msg = msg_parts[0]
else:
title = msg_parts[0]
msg = "\n\n".join(msg_parts[1:])

g = gh3.login(token=token)
repo = g.repository(org, repository)
repo.create_pull(title, "master", branch, msg)


def fetch_student(org, course, student, directory, token=None):
"""Fetch course repository for `student` from `org`

The repository will be cloned into a sub-directory in `directory`.

Returns the directory in which to find the students work.
"""
# use ssh if there is no token
if token is None:
fetch_command = [
"git",
"clone",
"[email protected]:{}/{}-{}.git".format(org, course, student),
]
else:
fetch_command = [
"git",
"clone",
"https://{}@github.com/{}/{}-{}.git".format(
token, org, course, student
),
]
subprocess.run(
fetch_command,
cwd=directory,
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)

return os.path.join(directory, "{}-{}".format(course, student))
#
# def check_student_repo_exists(org, course, student, token=None):
# """Check if the student has a repository for the course.
#
# It happens that students delete their repository or do not accept the
# invitation to the course. In either case they will not have a repository
# yet.
# """
# # temporarily change log level of github3.py as it prints weird messages
# # XXX could be done more nicely with a context manager maybe
# gh3_log = logging.getLogger("github3")
# old_level = gh3_log.level
# gh3_log.setLevel("ERROR")
#
# try:
# g = gh3.login(token=token)
# repository = "{}-{}".format(course, student)
# g.repository(org, repository)
#
# # TODO: this raises github3.exceptions.NotFoundError: 404 Not Found
# # It might be better to capture the specific exception and raise a more
# # helpful error?
# except Exception as e:
# raise e
#
# finally:
# gh3_log.setLevel(old_level)
#
#
# def close_existing_pullrequests(
# org, repository, branch_base="new-material-", token=None
# ):
# """Close all oustanding course material update Pull Requests
#
# If there are any PRs open in a student's repository that originate from
# a branch starting with `branch_base` as name and created by the user
# we are logged in we close them.
# """
# g = gh3.login(token=token)
# me = g.me()
# repo = g.repository(org, repository)
# for pr in repo.pull_requests(state="open"):
# origin = pr.head.label
# origin_repo, origin_branch = origin.split(":")
# if origin_branch.startswith(branch_base) and pr.user == me:
# pr.create_comment(
# "Closed in favor of a new Pull Request to "
# "bring you up-to-date."
# )
# pr.close()
#
#
# def create_pr(org, repository, branch, message, token):
# """Create a Pull Request with changes from branch"""
# msg_parts = message.split("\n\n")
# if len(msg_parts) == 1:
# title = msg = msg_parts[0]
# else:
# title = msg_parts[0]
# msg = "\n\n".join(msg_parts[1:])
#
# g = gh3.login(token=token)
# repo = g.repository(org, repository)
# repo.create_pull(title, "master", branch, msg)
#
#
# def fetch_student(org, course, student, directory, token=None):
# """Fetch course repository for `student` from `org`
#
# The repository will be cloned into a sub-directory in `directory`.
#
# Returns the directory in which to find the students work.
# """
# # use ssh if there is no token
# if token is None:
# fetch_command = [
# "git",
# "clone",
# "[email protected]:{}/{}-{}.git".format(org, course, student),
# ]
# else:
# fetch_command = [
# "git",
# "clone",
# "https://{}@github.com/{}/{}-{}.git".format(
# token, org, course, student
# ),
# ]
# subprocess.run(
# fetch_command,
# cwd=directory,
# check=True,
# stdout=subprocess.PIPE,
# stderr=subprocess.PIPE,
# )
#
# return os.path.join(directory, "{}-{}".format(course, student))
1 change: 0 additions & 1 deletion abcclassroom/tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import pytest

from pathlib import Path

import abcclassroom.config as abcconfig


Expand Down
Loading