diff --git a/eden/scm/sapling/ext/github/github_gh_cli.py b/eden/scm/sapling/ext/github/github_gh_cli.py index d87248f6c8dea..593493bf06e1c 100644 --- a/eden/scm/sapling/ext/github/github_gh_cli.py +++ b/eden/scm/sapling/ext/github/github_gh_cli.py @@ -16,32 +16,43 @@ JsonDict = Dict[str, Any] +# Scalar value that can be passed as a field to `gh api`. +_ScalarParam = Union[str, int, bool] +# `gh api` also supports array fields via repeated `key[]=value` args. +ParamValue = Union[_ScalarParam, List[_ScalarParam]] + async def make_request( - params: Dict[str, Union[str, int, bool]], + params: Dict[str, ParamValue], hostname: str, endpoint="graphql", method: Optional[str] = None, + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: """If successful, returns a Result whose value is parsed JSON returned by the request. """ - return await _make_request(params, hostname, endpoint, method) + return await _make_request(params, hostname, endpoint, method, headers) # Unexported extension/mock point. async def _make_request( - params: Dict[str, Union[str, int, bool]], + params: Dict[str, ParamValue], hostname: str, endpoint: str, method: Optional[str], + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: if method: endpoint_args = ["-X", method.upper(), endpoint] else: endpoint_args = [endpoint] + header_args = list( + itertools.chain(*[["-H", f"{k}: {v}"] for (k, v) in (headers or {}).items()]) + ) args = ( ["gh", "api", "--hostname", hostname] + + header_args + endpoint_args + list(itertools.chain(*[_format_param(k, v) for (k, v) in params.items()])) ) @@ -82,7 +93,38 @@ async def _make_request( ) -def _format_param(key: str, value: Union[str, int, bool]) -> List[str]: +def _format_param(key: str, value: ParamValue) -> List[str]: + r"""Formats a param as a list of arguments to pass to `gh api`. + + >>> _format_param("body", "hello") + ['-f', 'body=hello'] + >>> _format_param("number", 42) + ['-F', 'number=42'] + >>> _format_param("draft", True) + ['-F', 'draft=true'] + + Array values use the `gh api` repeated-field syntax, e.g. + `-F "pull_requests[]=101" -F "pull_requests[]=102"`: + + >>> _format_param("pull_requests", [101, 102]) + ['-F', 'pull_requests[]=101', '-F', 'pull_requests[]=102'] + >>> _format_param("labels", ["bug", "help wanted"]) + ['-f', 'labels[]=bug', '-f', 'labels[]=help wanted'] + + An empty array is passed explicitly as `key[]` without a value (per the + `gh api` manual) rather than dropped: an empty array and a missing field + can have different meanings to an endpoint: + + >>> _format_param("empty", []) + ['-F', 'empty[]'] + """ + if isinstance(value, list): + if not value: + return ["-F", f"{key}[]"] + return list( + itertools.chain(*[_format_param(f"{key}[]", v) for v in value]) + ) + # In Python, bool is a subclass of int, so check it first. if isinstance(value, bool): opt = "-F" diff --git a/eden/scm/sapling/ext/github/mock_utils.py b/eden/scm/sapling/ext/github/mock_utils.py index added9869489b..06e31e5452888 100644 --- a/eden/scm/sapling/ext/github/mock_utils.py +++ b/eden/scm/sapling/ext/github/mock_utils.py @@ -28,8 +28,11 @@ REPO_ID = "R_test_github_repo" USER_NAME = "facebook_username" -ParamsType = Dict[str, Union[bool, int, str]] -MakeRequestType = Callable[[ParamsType, str, str, Optional[str]], Result[JsonDict, str]] +ParamsType = Dict[str, Union[bool, int, str, List[bool], List[int], List[str]]] +MakeRequestType = Callable[ + [ParamsType, str, str, Optional[str], Optional[Dict[str, str]]], + Result[JsonDict, str], +] RunGitCommandType = Callable[[List[str], str], bytes] @@ -75,6 +78,7 @@ class MockGitHubServer: def __init__(self, hostname: str = GITHUB_HOSTNAME): self.hostname: str = hostname self.requests: Dict[str, MockRequest] = {} + self._consumed_keys: set = set() async def make_request( self, @@ -83,10 +87,15 @@ async def make_request( hostname: str, endpoint: str = "graphql", method: Optional[str] = None, + headers: Optional[Dict[str, str]] = None, ) -> Result[JsonDict, str]: """Wrapper function for `github_gh_cli.make_request`. It reads mock data from `self.requests` instead of sending network requests. + + Note that `headers` is intentionally not part of the request key: the + headers we send (e.g., X-GitHub-Api-Version) do not affect which mock + response should be returned. """ assert real_make_request.__name__ == "_make_request", ( f"expected '_make_request', but got '{real_make_request.__name__}'" @@ -96,11 +105,45 @@ async def make_request( if key not in self.requests: raise MockRequestNotFound(key, self.requests) + self._consumed_keys.add(key) return self.requests[key].get_response() def _add_request(self, request_key: str, request: "MockRequest") -> None: self.requests[request_key] = request + def unconsumed_requests(self) -> List[str]: + """Keys of expectations that were never requested.""" + return sorted(k for k in self.requests if k not in self._consumed_keys) + + def report_unconsumed(self, ui) -> None: + """Prints a warning for every expectation that was never requested. + + Intended to be called after the command under test has finished (see + wrap_with_consumption_check). Tests that use this produce no extra + output when all expectations were consumed, so any warning makes the + test fail: this catches code paths that silently stopped making a + request the test author expected. + """ + for key in self.unconsumed_requests(): + first_line = key.splitlines()[0] + ui.status_err(f"warning, unconsumed mock request: {first_line}\n") + + +def wrap_with_consumption_check(server: "MockGitHubServer", module, funcname) -> None: + """Wraps `module.funcname` (a command function taking `ui` as its first + argument) so that unconsumed mock expectations are reported after the + command finishes, even if it aborts. + """ + from sapling import extensions + + def wrapped(orig, ui, *args, **kwargs): + try: + return orig(ui, *args, **kwargs) + finally: + server.report_unconsumed(ui) + + extensions.wrapfunction(module, funcname, wrapped) + def expect_get_repository_request( self, owner: str = OWNER, name: str = REPO_NAME ) -> "GetRepositoryRequest": diff --git a/eden/scm/tests/test-doctest.py b/eden/scm/tests/test-doctest.py index e0f5d40b354df..90d1726ff9426 100644 --- a/eden/scm/tests/test-doctest.py +++ b/eden/scm/tests/test-doctest.py @@ -32,6 +32,7 @@ def testmod(name, optionflags=0, testtarget=None): testmod("sapling.pathlog") testmod("sapling.ext.github.archive_commit") +testmod("sapling.ext.github.github_gh_cli") testmod("sapling.ext.github.github_repo_util") testmod("sapling.ext.github.pr_parser") testmod("sapling.ext.github.pull_request_arg")