Posting to the iterations endpoint when mercurial is not accessible should return a sensible response

RESOLVED FIXED

Status

Conduit
General
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

Details

(URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

7 months ago
- Create mountebank stubs for operations needed to talk to the mercurial web API.
- Create exceptions dealing with mercurial web API errors such as improper data sent to the API or general unavailability of the mercurial server.
- Create test cases that test each of the different error conditions and generate the correct exceptions for each.

AC:

- commitindex /iterations/ API returns well formed error when mercurial server is unavailable or mercurial api generated application error.
(Assignee)

Comment 2

7 months ago
Notes:

•smacleod> dkl: http://gregoryszorc.com/blog/2015/08/18/json-apis-on-hg.mozilla.org/
•smacleod> e.g. https://hg.mozilla.org/automation/conduit/json-changelog/
•smacleod> we'll want to use the json responses for a lot of stuff
•smacleod> raw-rev probably for fetching diffs: https://hg.mozilla.org/automation/conduit/raw-rev/933f4dbc8b55
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8852077 - Flags: review?(mars)
Attachment #8852078 - Flags: review?(mars)
Attachment #8852079 - Flags: review?(dwalsh)
Attachment #8852080 - Flags: review?(dwalsh)

Comment 7

7 months ago
mozreview-review
Comment on attachment 8852079 [details]
mercurial: Create mercurial.py module for handling various data requests to hg instance (bug 1349673)

https://reviewboard.mozilla.org/r/124326/#review126908

::: commitindex/commitindex/commits/mercurial.py:23
(Diff revision 1)
> +    TODO:
> +    1. Load API URL from system wide config
> +    """
> +
> +    def __init__(self, rest_url):
> +        self.rest_url = rest_url

Should we use `@property` for `rest_url` as we do here?

https://reviewboard.mozilla.org/r/123512/diff/5

Comment 8

7 months ago
mozreview-review
Comment on attachment 8852080 [details]
commit-index: Mountebank tests for testing getting commit data and raw diff from hg server (bug 1349673)

https://reviewboard.mozilla.org/r/124328/#review126966

::: commitindex/commitindex/api/iterations.py:56
(Diff revision 1)
> +    1. host variable should go away and be pulled in from some
> +    configuration file or environment variable. It is added here
> +    to facilitate mountebank testing.
> +    """
> +
> +    mercurial = Mercurial(host)

This would be better as a factory function with tests, same as we have for the `Bugzilla` client code in triggers.py.

::: commitindex/commitindex/api/iterations.py:76
(Diff revision 1)
> +    1. host variable should go away and be pulled in from some
> +    configuration file or environment variable. It is added here
> +    to facilitate mountebank testing.
> +    """
> +
> +    mercurial = Mercurial(host)

See above re: factory function to get the client.

::: commitindex/tests/test_hg_server.py:185
(Diff revision 1)
> +    mercurial.create_commit_stubs(commit_data)
> +
> +    return commit_data
> +
> +
> +@pytest.mark.mercurial

I don't think we need the custom markers.  We can add custom markers later when we want to differentiate between different test execution environments.

::: commitindex/tests/test_hg_server.py:191
(Diff revision 1)
> +def test_get_commit_data_success(mercurial):
> +    """
> +    Tests for successfully getting extended commit data from Mercurial.
> +    """
> +
> +    commit_data = configure_test_stubs(mercurial)

If we want to keep calling `configure_test_stubs()` as a function and not a fixture, I suggest removing the `@pytest.fixture` decorator.

Alternatively we could rename/tweak `configure_test_stubs()` and use it as a fixture:

```

@pytest.fixture
def fake_commit_data(mercurial):
    # existing configure_test_stubs body goes here
    ....


def test_get_commit_data_success(mercurial, fake_commit_data):
    # fake_commit_data has same value as configure_test_stubs().
    ...
```

::: commitindex/tests/test_hg_server.py:212
(Diff revision 1)
> +    try:
> +        fetch_commit_data(
> +            ['12345'],
> +            mercurial.url,
> +            'automation/conduit',
> +        )
> +    except MercurialError as error:
> +        assert error.fault_code == 404

Have a look at [pytest.raises()](http://doc.pytest.org/en/latest/assert.html#assertions-about-expected-exceptions) for a helper that will print better error messages and can make the code smaller and more concise.

::: commitindex/tests/test_hg_server.py:221
(Diff revision 1)
> +    # Bad repo path
> +    try:
> +        fetch_commit_data(
> +            ['f5279c5bcc6c74e9cf767fad36930e9ae7d09bdc'], mercurial.url,
> +            'automation/bugzilla'
> +        )
> +    except MercurialError as error:
> +        assert error.fault_code == 404

This should be its own test, `test_fetch_data_with_bad_repo_path_raises_error()`.

::: commitindex/tests/test_hg_server.py:227
(Diff revision 1)
> +    try:
> +        fetch_commit_data(
> +            ['f5279c5bcc6c74e9cf767fad36930e9ae7d09bdc'], mercurial.url,
> +            'automation/bugzilla'
> +        )
> +    except MercurialError as error:

See above comment about `pytest.raises()`.

::: commitindex/tests/test_hg_server.py:266
(Diff revision 1)
> +            'automation/conduit',
> +        )
> +    except MercurialError as error:
> +        assert error.fault_code == 404
> +
> +    # Bad repo path

This should be its own test, `test_fetch_diff_with_bad_repo_path_raises_error()`.

Comment 9

7 months ago
mozreview-review
Comment on attachment 8852077 [details]
commit-index: make sure client sends 'Accept: application/json' to mountebank (bug 1349673)

https://reviewboard.mozilla.org/r/124322/#review128538
Attachment #8852077 - Flags: review?(mars) → review+

Comment 10

7 months ago
mozreview-review
Comment on attachment 8852077 [details]
commit-index: make sure client sends 'Accept: application/json' to mountebank (bug 1349673)

https://reviewboard.mozilla.org/r/124322/#review128540

::: commit-message-80b54:1
(Diff revision 1)
> +- make sure client sends 'Accept: application/json' to mountebank

The commit message needs a component.

Comment 11

7 months ago
mozreview-review
Comment on attachment 8852078 [details]
bugzilla: Fault string and fault code needed to be reversed (bug 1349673)

https://reviewboard.mozilla.org/r/124324/#review128544

::: commit-message-bce46:1
(Diff revision 1)
> +- Fault string and fault code needed to be reversed

The commit message needs a component.
Attachment #8852078 - Flags: review?(mars) → review+

Comment 12

7 months ago
mozreview-review
Comment on attachment 8852079 [details]
mercurial: Create mercurial.py module for handling various data requests to hg instance (bug 1349673)

https://reviewboard.mozilla.org/r/124326/#review126964

::: commitindex/commitindex/commits/mercurial.py:23
(Diff revision 1)
> +    TODO:
> +    1. Load API URL from system wide config
> +    """
> +
> +    def __init__(self, rest_url):
> +        self.rest_url = rest_url

This issue can be dropped since we changed approach and removed the @property code.
Attachment #8852079 - Flags: review?(mars) → review+

Comment 13

7 months ago
mozreview-review
Comment on attachment 8852080 [details]
commit-index: Mountebank tests for testing getting commit data and raw diff from hg server (bug 1349673)

https://reviewboard.mozilla.org/r/124328/#review128546

::: commitindex/commitindex/api/iterations.py:56
(Diff revision 1)
> +    1. host variable should go away and be pulled in from some
> +    configuration file or environment variable. It is added here
> +    to facilitate mountebank testing.
> +    """
> +
> +    mercurial = Mercurial(host)

opening an issue for my previous comment

::: commitindex/commitindex/api/iterations.py:76
(Diff revision 1)
> +    1. host variable should go away and be pulled in from some
> +    configuration file or environment variable. It is added here
> +    to facilitate mountebank testing.
> +    """
> +
> +    mercurial = Mercurial(host)

opening an issue for my previous comment

::: commitindex/tests/test_hg_server.py:185
(Diff revision 1)
> +    mercurial.create_commit_stubs(commit_data)
> +
> +    return commit_data
> +
> +
> +@pytest.mark.mercurial

opening an issue for my previous comment

::: commitindex/tests/test_hg_server.py:191
(Diff revision 1)
> +def test_get_commit_data_success(mercurial):
> +    """
> +    Tests for successfully getting extended commit data from Mercurial.
> +    """
> +
> +    commit_data = configure_test_stubs(mercurial)

opening an issue for my previous comment

::: commitindex/tests/test_hg_server.py:212
(Diff revision 1)
> +    try:
> +        fetch_commit_data(
> +            ['12345'],
> +            mercurial.url,
> +            'automation/conduit',
> +        )
> +    except MercurialError as error:
> +        assert error.fault_code == 404

opening an issue for my previous comment

::: commitindex/tests/test_hg_server.py:221
(Diff revision 1)
> +    # Bad repo path
> +    try:
> +        fetch_commit_data(
> +            ['f5279c5bcc6c74e9cf767fad36930e9ae7d09bdc'], mercurial.url,
> +            'automation/bugzilla'
> +        )
> +    except MercurialError as error:
> +        assert error.fault_code == 404

opening an issue for my previous comment

::: commitindex/tests/test_hg_server.py:227
(Diff revision 1)
> +    try:
> +        fetch_commit_data(
> +            ['f5279c5bcc6c74e9cf767fad36930e9ae7d09bdc'], mercurial.url,
> +            'automation/bugzilla'
> +        )
> +    except MercurialError as error:

opening an issue for my previous comment
Attachment #8852080 - Flags: review?(mars) → review-

Updated

7 months ago
Assignee: nobody → dkl
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

7 months ago
mozreview-review
Comment on attachment 8852080 [details]
commit-index: Mountebank tests for testing getting commit data and raw diff from hg server (bug 1349673)

https://reviewboard.mozilla.org/r/124328/#review129482

r+ with a refactoring

::: commitindex/commitindex/commits/mercurial.py:20
(Diff revision 2)
> -        self.rest_url = rest_url
> +        self._base_url = rest_url
>          self.session = requests.Session()
>  
> +    @property
> +    def rest_url(self):
> +        assert self._base_url is not None
> +        return self._base_url
> +
> +    @rest_url.setter
> +    def rest_url(self, value):
> +        self._base_url = value
> +

You don't need `rest_url` to be a property if you are using a factory function to construct mercurial client objects.

It looks like having this as a property was borrowed from an early approach to the Bugzilla client code that managed the client objects as singletons. We changed to a better approach and now build clients with a factory function.
Attachment #8852080 - Flags: review?(mars) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

7 months ago
mozreview-review-reply
Comment on attachment 8852078 [details]
bugzilla: Fault string and fault code needed to be reversed (bug 1349673)

https://reviewboard.mozilla.org/r/124324/#review128544

> The commit message needs a component.

Not an issue that blocks landing this time, but you should probably make the component "commitindex" next time.  Even though it's bugzilla client code, it's still part of the commitindex subproject.

Comment 24

7 months ago
Pushed by mfogels@mozilla.com:
https://hg.mozilla.org/automation/conduit/rev/02c2d25c43c3
commit-index: make sure client sends 'Accept: application/json' to mountebank r=mars
https://hg.mozilla.org/automation/conduit/rev/5c858a2e8a33
bugzilla: Fault string and fault code needed to be reversed r=mars
https://hg.mozilla.org/automation/conduit/rev/afe01cab1f55
mercurial: Create mercurial.py module for handling various data requests to hg instance r=mars
https://hg.mozilla.org/automation/conduit/rev/8ce1818ba623
commit-index: Mountebank tests for testing getting commit data and raw diff from hg server r=mars
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.