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

chore: Move tests to tests/ dir #60

Merged
merged 5 commits into from
Mar 4, 2024
Merged
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
24 changes: 10 additions & 14 deletions .github/workflows/pull-request.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,21 @@ jobs:
with:
python-version: ${{ matrix.python-version }}
- run: make deps
- run: make install
- run: make test
# Run coverage on one of the builds, doesn't matter which
- if: ${{ matrix.python-version == '3.12' }}
run: make cov-xml
- if: ${{ matrix.python-version == '3.12' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to keep - if: always() here. Otherwise, coverage won't report if make cov-xml fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think you want if: always because it's only running on one branch of the matrix. And anyway, if make cov-xml fails, how can coverage report?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, should it be...?

- if: ${{ !cancelled() && matrix.python-version == '3.12' }}

https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions

make cov-xml will fail if the coverage is below report requirements but will still report the coverage. We want this step to run regardless so that the coverage report is added to the Pull Request.

Here is an example of the behavior: #67

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, got it. Thanks for the demonstration.

Would it be better to just remove fail_under = 80 from .coveragerc? I'm not sure it's helping us more than having the coverage report on the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Either works for me.

uses: orgoro/coverage@v3.1
with:
coverageFile: coverage.xml
thresholdAll: 0.8
token: ${{ secrets.GITHUB_TOKEN }}
lint:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
- run: make deps
- run: make lint
cov:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
- run: make deps
- run: make test
- run: make cov-xml
- if: always()
uses: orgoro/coverage@v3.1
with:
coverageFile: coverage.xml
thresholdAll: 0.8
token: ${{ secrets.GITHUB_TOKEN }}
5 changes: 2 additions & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ Source = "https://github.com/posit-dev/posit-sdk-py"
Issues = "https://github.com/posit-dev/posit-sdk-py/issues"

[tool.pytest.ini_options]
python_files = [
"*_test.py",
"*_test_*.py",
addopts = [
"--import-mode=importlib",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you verified that this gives you the functionality you want?

https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html?highlight=import%20mode#import-modes

One drawback however is that test modules are non-importable by each other. Also, utility modules in the tests directories are not automatically importable because the tests directory is no longer added to sys.path.

If I understand this correctly, this means that abstracting mocked classes into their own module won't work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have not verified. I was just blindly following the pytest docs recommendation: "For new projects, we recommend to use importlib import mode." Happy to remove if/when it's a problem.

]

[tool.setuptools_scm]
Expand Down
30 changes: 0 additions & 30 deletions src/posit/connect/hooks_test.py

This file was deleted.

40 changes: 0 additions & 40 deletions src/posit/connect/hooks_test_deps.py

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from unittest.mock import MagicMock, Mock, patch

from .auth import Auth
from posit.connect.auth import Auth


class TestAuth:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

from unittest.mock import MagicMock, patch

from .client import Client
from posit.connect import Client


@pytest.fixture
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from unittest.mock import patch

from .config import Config, _get_api_key, _get_url
from posit.connect.config import Config, _get_api_key, _get_url


@patch.dict("os.environ", {"CONNECT_API_KEY": "foobar"})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import responses

from .client import Client
from posit.connect.client import Client


class TestContents:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import pytest

from .errors import ClientError
from posit.connect.errors import ClientError


class TestClientError:
Expand Down
65 changes: 65 additions & 0 deletions tests/posit/connect/test_hooks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import io

import pytest

from requests import HTTPError, Response
from unittest.mock import Mock, patch

from posit.connect.errors import ClientError
from posit.connect.hooks import handle_errors


def test_success():
response = Mock()
response.status_code = 200
assert handle_errors(response) == response


def test_client_error():
response = Mock()
response.status_code = 400
response.json = Mock(return_value={"code": 0, "error": "foobar"})
with pytest.raises(ClientError):
handle_errors(response)


@patch("posit.connect.hooks.JSONDecodeError")
def test_client_error_without_payload(JSONDecodeError):
response = Mock()
response.status_code = 404
response.json = Mock(side_effect=JSONDecodeError())
response.raise_for_status = Mock(side_effect=Exception())
with pytest.raises(Exception):
handle_errors(response)


def test_200():
response = Response()
response.status_code = 200
assert handle_errors(response) == response


def test_response_client_error_with_plaintext_payload():
response = Response()
response.status_code = 404
response.raw = io.BytesIO(b"Plain text 404 Not Found")
with pytest.raises(HTTPError):
handle_errors(response)


def test_response_client_error_with_json_payload():
response = Response()
response.status_code = 400
response.raw = io.BytesIO(b'{"code":0,"error":"foobar"}')
with pytest.raises(
ClientError, match=r"foobar \(Error Code: 0, HTTP Status: 400 Bad Request\)"
):
handle_errors(response)


def test_response_client_error_without_payload():
response = Response()
response.status_code = 404
response.raw = io.BytesIO(b"Plain text 404 Not Found")
with pytest.raises(HTTPError):
handle_errors(response)
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import responses

from .client import Client
from posit.connect import Client


class TestOAuthIntegrations:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import pytest

from .urls import append_path, server_to_api_url, validate
from posit.connect.urls import append_path, server_to_api_url, validate


def test_append_path():
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import pandas as pd
import responses

from .client import Client
from posit.connect.client import Client


class TestUsers:
Expand Down
Loading