Skip to content

Convert CLI tests to integration tests, increase overall test coverage, slight change to API url building logistics #385

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

Merged
merged 14 commits into from
Feb 4, 2022

Conversation

jreiberkyle
Copy link
Contributor

@jreiberkyle jreiberkyle commented Feb 1, 2022

Changes:

  • CLI tests are now integration tests, where none of the planet library is mocked, just server responses are mocked. While many of the integration tests are 1:1 with the unit tests, some new tests were added and some existing tests were broken into multiple integration tests. Overall coverage increased.
  • Minimum test coverage to pass tests is increased from 95% to 97%
  • Change where slashes are in API url building to match API documentation (example pulled from docs below, note how relative paths start with a slash)
    image
  • URLs are now built using f-strings instead of string concatenation (closes move url creation in the auth and orders clients to f-strings #382)

Closes #327

… remove ref to auth in example

The orders API includes multiple api endpoints. They all begin with
'https://api.planet.com/' but then the relative paths are:
'compute/ops/stats/orders/v2/', 'compute/ops/orders/v2/', and
'compute/ops/bulk/orders/v2/'. The auth API endpoint begins with the
same url and the relative path is 'v0/auth/'. Setting the base url for
auth to 'https://api.planet.com/v0/auth/' makes sense for the auth
client, but for the orders client we cannot use one of the relative
paths for the base url. We could use `compute/ops` but that doesn't
actually get us to the orders API endpoint. Backing off to just have the
base url be 'https://api.planet.com/' seems less confusing than having
the base url for the orders API be some portion of the full path to the
endpoint and having the base url for the auth API be the full path.
@jreiberkyle jreiberkyle linked an issue Feb 1, 2022 that may be closed by this pull request
@jreiberkyle jreiberkyle changed the title WIP: Convert CLI tests to integration tests, change base_url handling, increase overall test coverage Convert CLI tests to integration tests, increase overall test coverage, slight change to API url building logistics Feb 2, 2022
@jreiberkyle jreiberkyle self-assigned this Feb 2, 2022
Comment on lines 21 to 26
@pytest.mark.parametrize(
"pretty,expected",
[(False, '{"key": "val"}'), (True, '{\n "key": "val"\n}')])
def test_cli_echo_json(pretty, expected, monkeypatch):
mock_echo = Mock()
monkeypatch.setattr(io.click, 'echo', mock_echo)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jreiberkyle what would you think about using mock.patch instead of pytest's monkeypatch?

Suggested change
@pytest.mark.parametrize(
"pretty,expected",
[(False, '{"key": "val"}'), (True, '{\n "key": "val"\n}')])
def test_cli_echo_json(pretty, expected, monkeypatch):
mock_echo = Mock()
monkeypatch.setattr(io.click, 'echo', mock_echo)
@pytest.mark.parametrize(
"pretty,expected",
[(False, '{"key": "val"}'), (True, '{\n "key": "val"\n}')])
@mock.patch('io.click.echo')
def test_cli_echo_json(mock_echo, pretty, expected, monkeypatch):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works beautifully with one change: patch uses the full path name to the module. (see paragraph discussing the target arg in the patch docs)

Copy link
Contributor

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

I left one suggestion inline for using mock.patch instead of pytest.monkeypatch in the case where we're using mock.Mock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert CLI tests to integration tests
2 participants