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

Fix GitHubAPI fakes verification tests #4727

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion TESTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ We also provide DRF's testing tools as fixtures to mirror pytest-django.
We use a fake object, `FakeGitHubAPI`, to test our uses of GitHub's API.
However, we want to [verify that fake](https://pythonspeed.com/articles/verified-fakes/), so we have a set of verification tests in `tests/verification/`.

`test-verification` will run those tests, but `just test` and `just test-ci` will not (see [ADR#21](docs/adr/0021-move-verification-tests.md) for more details).
`test-verification` will run those tests marked with `pytest.mark.verification`, but `just test` and `just test-ci` will not (see [ADR#21](docs/adr/0021-move-verification-tests.md) for more details).
This way, the verification tests verify our faster tests are correct.
We have a separate GitHub org, `opensafely-testing`, and bot user, `opensafely-testing-bot`, for performing these tests.
We use a different env var, `GITHUB_TOKEN_TESTING`, to pass the required PAT in.
Expand Down
2 changes: 1 addition & 1 deletion tests/fakes.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ def get_issue_number_from_title(
raise GitHubError()

def create_issue_comment(
self, org, repo, title_text, body, latest=True, issue_number=1
self, org, repo, title_text, body, latest=True, issue_number=None
):
# Some unit tests want to check the message.
raise GitHubError("An error occurred")
Expand Down
18 changes: 15 additions & 3 deletions tests/verification/test_github.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import inspect

import pytest
import requests.exceptions
import stamina
Expand All @@ -7,6 +9,7 @@
from jobserver.github import (
ConnectionException,
GitHubAPI,
GitHubError,
HTTPError,
RepoAlreadyExists,
RepoNotYetCreated,
Expand All @@ -21,7 +24,6 @@
pytestmark = [pytest.mark.verification, pytest.mark.disable_db]


@pytest.mark.disable_db
class TestGitHubAPIAgainstFakes:
def test_fake_public_method_signatures(self):
"""Test that `FakeGitHubAPI` has the same public methods with the same
Expand All @@ -44,6 +46,18 @@ def test_fake_with_errors_public_method_signatures(self):
)


class TestFakeGitHubAPIWithErrors:
def test_public_methods_raise_githuberror(self):
fake = FakeGitHubAPIWithErrors
for name, method in inspect.getmembers(fake, predicate=inspect.isfunction):
if not name.startswith("_"):
sig = inspect.signature(method)
args = {param.name: None for param in sig.parameters.values()}

with pytest.raises(GitHubError):
method(**args)


@pytest.fixture
def clear_topics(github_api):
args = [
Expand All @@ -66,7 +80,6 @@ def github_api():
return GitHubAPI(token=Env().str("GITHUB_TOKEN_TESTING"))


@pytest.mark.disable_db
@pytest.mark.usefixtures("enable_network")
class TestGithubAPIPrivate:
"""Tests of the real GitHubAPI that require permissions on a private
Expand Down Expand Up @@ -229,7 +242,6 @@ def test_set_repo_topics(self, github_api, clear_topics):
assert repo["topics"] == ["testing"]


@pytest.mark.disable_db
@pytest.mark.usefixtures("enable_network")
class TestGithubAPINonPrivate:
"""Tests of the real GitHubAPI that don't require permissions on a private
Expand Down
4 changes: 0 additions & 4 deletions tests/verification/test_opencodelists.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
pytestmark = [pytest.mark.verification, pytest.mark.disable_db]


@pytest.mark.disable_db
def test_fake_public_method_signatures():
"""Test that `FakeOpenCodelistsAPI` has the same public methods with the
same signatures as the real one."""
Expand All @@ -25,7 +24,6 @@ def opencodelists_api():
return OpenCodelistsAPI()


@pytest.mark.disable_db
def test_get_codelists(enable_network, opencodelists_api):
args = ["snomedct"]

Expand All @@ -37,7 +35,6 @@ def test_get_codelists(enable_network, opencodelists_api):
assert real is not None


@pytest.mark.disable_db
def test_get_codelists_with_unknown_coding_system(enable_network, opencodelists_api):
args = ["test"]

Expand All @@ -49,7 +46,6 @@ def test_get_codelists_with_unknown_coding_system(enable_network, opencodelists_
assert real == []


@pytest.mark.disable_db
def test_check_codelists(enable_network, opencodelists_api):
args = ["{}", ""]

Expand Down
30 changes: 16 additions & 14 deletions tests/verification/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
from .utils import assert_public_method_signature_equality


@pytest.mark.disable_db
pytestmark = [pytest.mark.verification, pytest.mark.disable_db]


class TestPublicMethodSignatureEquality:
def test_identity(self):
"""Test when applied against the same class."""

class ClassA:
class ClassA: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...
Expand All @@ -20,13 +22,13 @@ def method_two(self, z): ...
def test_matching_methods(self):
"""Test when methods match between classes."""

class ClassA:
class ClassA: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...

# This class is identical to Class A.
class ClassB:
class ClassB: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...
Expand All @@ -36,13 +38,13 @@ def method_two(self, z): ...
def test_signature_mismatch(self):
"""Test when method signatures differ between classes."""

class ClassA:
class ClassA: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...

# The same as A, but a method has a different signature.
class ClassC:
class ClassC: # pragma: no cover - Test class that is never instantiated.
# Different signature to A.method_one.
def method_one(self, x): ...

Expand All @@ -54,13 +56,13 @@ def method_two(self, z): ...
def test_extra_method(self):
"""Test when the second class has an extra method."""

class ClassA:
class ClassA: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...

# The same as A, but with an extra method.
class ClassD:
class ClassD: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...
Expand All @@ -73,13 +75,13 @@ def method_three(self): ...
def test_missing_method(self):
"""Test when a method is missing in the second class."""

class ClassA:
class ClassA: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...

# The same as A, but with an extra method.
class ClassD:
class ClassD: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...
Expand All @@ -92,13 +94,13 @@ def method_three(self): ...
def test_ignore_methods(self):
"""Test when certain methods are ignored in the comparison."""

class ClassA:
class ClassA: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...

# The same as A, but with an extra method.
class ClassD:
class ClassD: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...
Expand All @@ -112,13 +114,13 @@ def method_three(self): ...
def test_ignore_private_methods(self):
"""Test that extra private methods are ignored."""

class ClassA:
class ClassA: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...

# The same as A, but with a private method.
class ClassE:
class ClassE: # pragma: no cover - Test class that is never instantiated.
def method_one(self, x, y): ...

def method_two(self, z): ...
Expand Down