Repository: conbench/conbench (language: python)

Issue:
rethink: magic of setting CI=true and _expected_docs
I just ran into a case where a test failed in CI which did not fail locally:

```
=========================== short test summary info ============================
FAILED conbench/tests/api/test_docs.py::TestDocs::test_docs - RuntimeError: The 'CI' env var was set so we're assuming this test was run in CI. However, /app/conbench/tests/api/_expected_docs.py was not updated with the latest docs changes. Run `pytest conbench/tests/api/test_docs.py` locally to fix the file automatically.
============= 1 failed, 383 passed, 2 skipped in 209.92s (0:03:29) =============
```

With developer productivity in mind let's see if we can synchronize this again. Maybe this was even a regression introduced by moving to the docker-compose-based method to invoke tests locally.





Discussion / hints:
That's right, I forgot about this. I typically run my local tests outside of docker-compose, so this hasn't been a problem for me. But because of #437, if you run local tests inside of docker-compose, the file doesn't get changed locally and so it can't be pushed.

With the current setup, the property that's most important to me is that PR reviewers can see the diff of how the OpenAPI docs might change along with a given code change. I don't really care how we get to that property, and agree that the current way has friction. 

I am intrigued by @jonkeane's [recent idea](https://github.com/conbench/conbench/pull/468#discussion_r1036258970) that we could trigger a bot to automatically update the PR with the expected doc changes. I like this opt-in method a little better than having the bot do it without being asked, because there's always a chance that a developer could cause a doc change that was unintended, and we wouldn't want that to slip through.

Other options are to keep the current setup and mount the repo to the local docker-compose container so that local changes are picked up, or get rid of the `_expected_docs.py` file altogether and have a bot otherwise attach the doc diff to the PR (maybe a comment or a GitHub Check or a build artifact similar to the webapp screenshots we're discussing in #483), or something else.
Ignoring bot magic for now, trying to get a better understanding of this actual challenge. I am rather confident that I have seen this type of challenge before (still need to confirm).

The most important principle I think is to make the local test suite have a signal that is strong: if it passes, it's very likely that CI passes. 

In that spirit, I think it's the right solution to make this test error out if `_expected_docs.py` does not match the output given by `/api/docs.json`. That should error out in any test environment (locally and in CI).

However, if it _does_ error out the error message should give a really useful hint for a developer for how to rebuild `_expected_docs.py`.

Then we can remove any magic from the test suite, and also are not faced with the challenge of requiring output of the test suite to land on the host file system.


This command embraces the docker-compose way of running things locally, and then uses `curl` to fetch the current docs.json -- and writes it to the host's file system:

```
docker-compose up --detach && \
    echo "using $(docker-compose port app 5000) to reach app" && \
    curl --silent --show-error --fail --retry 10 \
        --retry-all-errors --retry-delay 1 --retry-max-time 30 \
        http://$(docker-compose port app 5000)/api/docs.json > new_api_docs.json && \
    docker-compose down
```
(just to show the principle, that it can after all be easy to regenerate this with a dedicated chain of commands, and w/o running the test suite)

Should of course be abstracted away: https://github.com/conbench/conbench/issues/503

Fix patch:
diff --git a/Makefile b/Makefile
index cc7642557..0dbb67c5d 100644
--- a/Makefile
+++ b/Makefile
@@ -48,6 +48,11 @@ rebuild-expected-api-docs: run-app-in-background
 	git diff ./conbench/tests/api/_expected_docs.py
 
 
+.PHONY: run-app
+run-app:
+	docker compose down && docker compose up --build
+
+
 .PHONY: run-app-in-background
 run-app-in-background:
 	docker compose up --build --wait --detach


Test patch:
diff --git a/conbench/tests/api/test_docs.py b/conbench/tests/api/test_docs.py
index a60bf78e7..2a1e8c374 100644
--- a/conbench/tests/api/test_docs.py
+++ b/conbench/tests/api/test_docs.py
@@ -1,38 +1,43 @@
 import ast
+import logging
 import os
-import subprocess
 
 from openapi_spec_validator import validate_spec
 
 from ...tests.api import _asserts
 
+log = logging.getLogger(__name__)
+
 this_dir = os.path.abspath(os.path.dirname(__file__))
 
 
 class TestDocs(_asserts.ApiEndpointTest):
     def test_docs(self, client):
         response = client.get("/api/docs.json")
+
         path = os.path.join(this_dir, "_expected_docs.py")
 
         with open(path) as f:
             expected_docs = ast.literal_eval(f.read())
 
         try:
+            # Note(JP): it seems that the hope here is that `assert_200_ok`
+            # performs a deep object equality inspection, relying on the
+            # `assert r.json == expected` equality check in the implementaion
+            # of `assert_200_ok()`. But how deep is that, really? Typically,
+            # the safest way to make sure that all details are after all equal
+            # is to perform stable string serialization. What would we lose if
+            # we were to compare JSON documents here?
             self.assert_200_ok(response, expected_docs)
-        except AssertionError:
-            if os.getenv("CI"):
-                raise RuntimeError(
-                    "The 'CI' env var was set so we're assuming this test was run in "
-                    f"CI. However, {path} was not updated with the latest docs "
-                    "changes. Run `pytest conbench/tests/api/test_docs.py` locally to "
-                    "fix the file automatically."
-                )
-            # update expected docs on API changes
-            # (onus is on devs to review diff)
-            with open(path, "w") as f:
-                f.write(str(response.json))
-            subprocess.run(["black", path])
-
-        with open(path) as f:
-            expected_docs = ast.literal_eval(f.read())
+        except AssertionError as exc:
+            # Hoping that this shows a useful diff.
+            log.info("caught assertion error: %s", exc)
+            raise Exception(
+                "/api/docs.json and _expected_docs.py are out of sync. "
+                "Run `make rebuild-expected-api-docs` to regenerate "
+                "_expected_docs.py and then review the diff manually."
+            )
+
+        # Maybe this is where we need a Python object structure, and not just
+        # a JSON document.
         validate_spec(expected_docs)