Merge pull request #171 from plotly/andrew/test_encoder

Andrew/test encoder

.github/workflows/test.yml

@@ -24,14 +24,14 @@ jobs:
         if: ${{ ! runner.debug }}
         run:  xvfb-run pytest -W error -n auto -v -rfE --capture=fd tests/test_process.py
         timeout-minutes: 2
-      - name: Test The Rest DEBUG
-        if: runner.debug
-        run:  pytest -W error -vvv -rA --capture=tee-sys --ignore=tests/test_process.py
-        timeout-minutes: 5
       - name: Test The Rest
         if: ${{ ! runner.debug }}
         run:  pytest -W error -n auto -v -rfE --ignore=tests/test_process.py
         timeout-minutes: 2
+      - name: Test The Rest DEBUG
+        if: runner.debug
+        run:  pytest -W error -vvv -rA --capture=tee-sys --ignore=tests/test_process.py
+        timeout-minutes: 5
   test-windows:
     runs-on: windows-latest
     steps:
@@ -54,14 +54,14 @@ jobs:
         if: ${{ ! runner.debug }}
         run:  pytest -W error -n auto -v -rFe --capture=fd tests/test_process.py
         timeout-minutes: 2
-      - name: Test The Rest DEBUG
-        if: runner.debug
-        run:  pytest -W error -vvv -rA --capture=tee-sys --ignore=tests/test_process.py
-        timeout-minutes: 5
       - name: Test The Rest
         if: ${{ ! runner.debug }}
         run:  pytest -W error -n auto -v -rfE --ignore=tests/test_process.py
         timeout-minutes: 2
+      - name: Test The Rest DEBUG
+        if: runner.debug
+        run:  pytest -W error -vvv -rA --capture=tee-sys --ignore=tests/test_process.py
+        timeout-minutes: 5
   test-mac:
     runs-on: macos-latest
     steps:
@@ -84,12 +84,12 @@ jobs:
         if: ${{ ! runner.debug }}
         run:  pytest -W error -n auto -v -rFe --capture=fd tests/test_process.py
         timeout-minutes: 4
-      - name: Test The Rest DEBUG
-        if: runner.debug
-        run:  pytest -W error -vvv -rA --capture=tee-sys --ignore=tests/test_process.py
-        timeout-minutes: 5
       - name: Test The Rest
         if: ${{ ! runner.debug }}
         run:  pytest -W error -n auto -v -rfE --ignore=tests/test_process.py
         timeout-minutes: 3
+      - name: Test The Rest DEBUG
+        if: runner.debug
+        run:  pytest -W error -vvv -rA --capture=tee-sys --ignore=tests/test_process.py
+        timeout-minutes: 5
 

choreographer/pipe.py

@@ -47,18 +47,24 @@ def __init__(self, debug=False, json_encoder=MultiEncoder):
         # this is just a convenience to prevent multiple shutdowns
         self.shutdown_lock = Lock()
 
+    def serialize(self, obj):
+        message = simplejson.dumps(obj, ensure_ascii=False, ignore_nan=True, cls=self.json_encoder)
+        #if debug:
+        #    print(f"write_json: {message}", file=sys.stderr)
+            # windows may print weird characters if we set utf-8 instead of utf-16
+            # check this TODO
+        return message.encode("utf-8") + b"\0"
+
+    def deserialize(self, message):
+        return simplejson.loads(message)
+
     def write_json(self, obj, debug=None):
         if self.shutdown_lock.locked():
             raise PipeClosedError()
         if not debug: debug = self.debug
         if debug:
             print("write_json:", file=sys.stderr)
-        message = simplejson.dumps(obj, ensure_ascii=False, ignore_nan=True, cls=self.json_encoder)
-        encoded_message = message.encode("utf-8") + b"\0"
-        if debug:
-            print(f"write_json: {message}", file=sys.stderr)
-            # windows may print weird characters if we set utf-8 instead of utf-16
-            # check this TODO
+        encoded_message = self.serialize(obj)
         try:
             os.write(self.write_to_chromium, encoded_message)
         except OSError as e:
@@ -114,7 +120,7 @@ def read_jsons(self, blocking=True, debug=None):
         for raw_message in decoded_buffer.split("\0"):
             if raw_message:
                 try:
-                    jsons.append(simplejson.loads(raw_message))
+                    jsons.append(self.deserialize(raw_message))
                 except BaseException as e:
                     if debug:
                         print(f"Problem with {raw_message} in json: {e}", file=sys.stderr)

pyproject.toml

@@ -30,8 +30,10 @@ dev = [
   "pytest",
   "pytest-asyncio",
   "pytest-xdist",
-  "async-timeout"
-  ]
+  "async-timeout",
+  "poethepoet>=0.31.1",
+  "numpy"
+]
 
 [project.scripts]
 dtdoctor = "choreographer.browser:diagnose"
@@ -42,3 +44,21 @@ ignore = ["E701"]
 [tool.pytest.ini_options]
 asyncio_default_fixture_loop_scope = "function"
 
+[tool.poe.tasks]
+_test_proc = "pytest -W error -n auto -v -rfE --capture=fd tests/test_process.py"
+_test_fn = "pytest -W error -n auto -v -rfE --ignore=tests/test_process.py"
+_debug-test_proc = "pytest -W error -vvx -rA --capture=tee-sys tests/test_process.py"
+_debug-test_fn = "pytest -W error -vvvx -rA --capture=tee-sys --ignore=tests/test_process.py"
+
+# the capture on these is weird, the mechanics are weird, i forget exactly whats its doing
+[tool.poe.tasks.test]
+sequence = ["_test_proc", "_test_fn"]
+help = "Run all tests quickly"
+
+[tool.poe.tasks.debug-test]
+sequence = ["_debug-test_proc", "_debug-test_fn"]
+help = "Run test by test, slowly, quitting after first error"
+
+[tool.poe.tasks.filter-test]
+cmd = "pytest -W error -vvvx -rA --capture=tee-sys"
+help = "Run any/all tests one by one with basic settings: can include filename and -k filters"

tests/conftest.py

@@ -12,6 +12,9 @@
 # especially in fixtures- since they may buffer your outputs
 # and can freeze w/o dumping the buffer
 
+##### Parameterized Arguments
+# Are used to re-run tests under different conditions
+
 @pytest.fixture(params=[True, False], ids=["enable_sandbox", ""])
 def sandbox(request):
     return request.param
@@ -24,28 +27,26 @@ def gpu(request):
 def headless(request):
     return request.param
 
-
 @pytest.fixture(params=[True, False], ids=["debug", ""])
 def debug(request):
     return request.param
 
-
 @pytest.fixture(params=[True, False], ids=["debug_browser", ""])
 def debug_browser(request):
     return request.param
 
 
+# --headless is the default flag for most tests, but you can set --no-headless if you want to watch
 def pytest_addoption(parser):
     parser.addoption("--headless", action="store_true", dest="headless", default=True)
     parser.addoption("--no-headless", dest="headless", action="store_false")
 
-
+# browser fixture will supply a browser for you
 @pytest_asyncio.fixture(scope="function", loop_scope="function")
 async def browser(request):
-    # this needs also to be set by command line TODO
     headless = request.config.getoption("--headless")
     debug = request.config.get_verbosity() > 2
-    debug_browser = None if debug else False
+    debug_browser = None if debug else False # what's going on here
     browser = await choreo.Browser(
         headless=headless, debug=debug, debug_browser=debug_browser
     )
@@ -58,29 +59,18 @@ async def browser(request):
     if os.path.exists(temp_dir):
         raise RuntimeError(f"Temporary directory not deleted successfully: {temp_dir}")
 
-
-
-@pytest_asyncio.fixture(scope="function", loop_scope="function")
-async def browser_verbose():
-    browser = await choreo.Browser(debug=True, debug_browser=True)
-    temp_dir = browser._temp_dir_name
-    yield browser
-    await browser.close()
-    if os.path.exists(temp_dir):
-        raise RuntimeError(f"Temporary directory not deleted successfully: {temp_dir}")
-
-# add a 2 second timeout if there is a browser fixture
-# we do timeouts manually in test_process because it
-# does/tests its own browser.close()
+# add a timeout if tests requests browser
+# but if tests creates their own browser they are responsible
+# a fixture can be used to specify the timeout: timeout=10
+# else it uses pytest.default_timeout
 @pytest.hookimpl(tryfirst=True, hookwrapper=True)
 def pytest_runtest_setup(item: pytest.Item):
     yield
-
-    if "browser" in item.funcargs or "browser_verbose" in item.funcargs:
+    if "browser" in item.funcargs:
         raw_test_fn = item.obj
         timeouts = [k for k in item.funcargs if k.startswith("timeout")]
         timeout = item.funcargs[timeouts[-1]] if len(timeouts) else pytest.default_timeout
-        if item.get_closest_marker("asyncio") and timeout:
+        if item.get_closest_marker("asyncio") and timeout: # "closest" because markers can be function/session/package etc
             async def wrapped_test_fn(*args, **kwargs):
                 try:
                     return await asyncio.wait_for(
@@ -94,13 +84,12 @@ def pytest_configure():
     # change this by command line TODO
     pytest.default_timeout = 5
 
-# add this fixture to extend timeout
-# there is 6 second max test length for all
-# which kills all tests
-@pytest.fixture(scope="session")
-def timeout_long():
-    return 8
-
+# pytests capture-but-display mechanics for output are somewhat spaghetti
+# more information here:
+# https://github.com/pytest-dev/pytest/pull/12854
+# is in the note above, capture=no will cancel all capture but also allow all error
+# to bubble up without buffering, which is helpful since sometimes error break the
+# buffering mechanics
 @pytest.fixture(scope="function")
 def capteesys(request):
     from _pytest import capture

tests/test_browser.py

@@ -2,16 +2,11 @@
 
 import choreographer as choreo
 
-
-url = (
-    "",
-    "",
-)
-
+# We no longer use live URLs to as not depend on the network
 
 @pytest.mark.asyncio
 async def test_create_and_close_tab(browser):
-    tab = await browser.create_tab(url[0])
+    tab = await browser.create_tab("")
     assert isinstance(tab, choreo.tab.Tab)
     assert tab.target_id in browser.tabs
     await browser.close_tab(tab)
@@ -28,6 +23,8 @@ async def test_create_and_close_session(browser):
     assert session.session_id not in browser.sessions
 
 
+# Along with testing, this could be repurposed as well to diagnose
+# This deserves some thought re. difference between write_json and send_command
 @pytest.mark.asyncio
 async def test_browser_write_json(browser):
     # Test valid request with correct id and method
@@ -74,7 +71,7 @@ async def test_browser_write_json(browser):
     with pytest.raises(
         choreo.protocol.MessageTypeError,
     ):
-        await browser.write_json({"id": "2", "method": "Target.getTargets"})
+        await browser.write_json({"id": "string", "method": "Target.getTargets"})
 
 
 @pytest.mark.asyncio
@@ -96,7 +93,7 @@ async def test_browser_send_command(browser):
 
 @pytest.mark.asyncio
 async def test_populate_targets(browser):
-    await browser.send_command(command="Target.createTarget", params={"url": url[1]})
+    await browser.send_command(command="Target.createTarget", params={"url": ""})
     await browser.populate_targets()
     if browser.headless is False:
         assert len(browser.tabs) == 2
@@ -106,8 +103,8 @@ async def test_populate_targets(browser):
 
 @pytest.mark.asyncio
 async def test_get_tab(browser):
-    await browser.create_tab(url[0])
+    await browser.create_tab("")
     assert browser.get_tab() == list(browser.tabs.values())[0]
     await browser.create_tab()
-    await browser.create_tab(url[1])
+    await browser.create_tab("")
     assert browser.get_tab() == list(browser.tabs.values())[0]

tests/test_process.py

@@ -57,16 +57,15 @@ async def test_no_context(capteesys, headless, debug, debug_browser, sandbox, gp
     await asyncio.sleep(0)
     assert not os.path.exists(temp_dir)
 
-# we're basically just harrassing choreographer with a kill in this test to see if it behaves well
+# Harrass choreographer with a kill in this test to see if its clean in a way
+# tempdir may survive protected by chromium subprocess surviving the kill
 @pytest.mark.asyncio(loop_scope="function")
 async def test_watchdog(capteesys, headless, debug, debug_browser):
     browser = await choreo.Browser(
         headless=headless,
         debug=debug,
         debug_browser=None if debug_browser else False,
     )
-    #temp_dir = browser._temp_dir_name
-    #async with timeout(pytest.default_timeout):
 
     if platform.system() == "Windows":
         subprocess.call(
@@ -77,10 +76,9 @@ async def test_watchdog(capteesys, headless, debug, debug_browser):
     else:
         os.kill(browser.subprocess.pid, signal.SIGKILL)
     await asyncio.sleep(1.5)
+
     with pytest.raises((choreo.browser.PipeClosedError, choreo.browser.BrowserClosedError)):
-        await browser.send_command(command="Target.getTargets") # could check for error, for close
-    # interestingly, try/except doesn't work here? maybe problem with pytest
+        await browser.send_command(command="Target.getTargets")
 
     await browser.close()
     await asyncio.sleep(0)
-    # assert not os.path.exists(temp_dir) # since we slopily kill the browser, we have no idea whats going to happen

tests/test_serializer.py

@@ -0,0 +1,26 @@
+from datetime import datetime
+
+import numpy as np
+
+from choreographer.pipe import Pipe
+
+data = [1, 2.00, 3, float("nan"), float("inf"), float("-inf"), datetime(1970, 1, 1)]
+expected_message = b'[1, 2.0, 3, null, null, null, "1970-01-01T00:00:00"]\x00'
+converted_type = [int, float, int, type(None), type(None), type(None), str]
+
+def test_de_serialize():
+    pipe = Pipe()
+    message = pipe.serialize(data)
+    assert message == expected_message
+    obj = pipe.deserialize(message[:-1]) # split out \0
+    for o, t in zip(obj, converted_type):
+        assert isinstance(o, t)
+    message_np = pipe.serialize(np.array(data))
+    assert message_np == expected_message
+    obj_np = pipe.deserialize(message_np[:-1]) # split out \0
+    for o, t in zip(obj_np, converted_type):
+        assert isinstance(o, t)
+
+
+# TODO: Not sure if this all the data we have to worry about:
+# we should also run through mocks and have it print data.

tests/test_tab.py

@@ -5,7 +5,7 @@
 
 import choreographer as choreo
 
-
+# this ignores extra stuff in received- only that we at least have what is expected
 def check_response_dictionary(response_received, response_expected):
     for k, v in response_expected.items():
         if isinstance(v, dict):