diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 17f0fc5f5..657d7749a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -27,7 +27,7 @@ jobs: run: pip install -r requirements/nox-deps.txt - name: Run Tests env: { "CI": "true" } - run: nox -s test_python_suite -- --headless + run: nox -s test_python_suite -- --headless --maxfail=3 test-python-environments: runs-on: ${{ matrix.os }} strategy: @@ -48,7 +48,7 @@ jobs: run: pip install -r requirements/nox-deps.txt - name: Run Tests env: { "CI": "true" } - run: nox -s test_python -- --headless --no-cov + run: nox -s test_python --stop-on-first-error -- --headless --maxfail=3 --no-cov test-docs: runs-on: ubuntu-latest steps: diff --git a/noxfile.py b/noxfile.py index c55a7515f..4e868cc90 100644 --- a/noxfile.py +++ b/noxfile.py @@ -181,6 +181,8 @@ def test_python_suite(session: Session) -> None: install_requirements_file(session, "test-env") posargs = session.posargs + posargs += ["--reruns", "3", "--reruns-delay", "1"] + if "--no-cov" in session.posargs: session.log("Coverage won't be checked") session.install(".[all]") diff --git a/requirements/pkg-deps.txt b/requirements/pkg-deps.txt index 1fc2e4ab9..f4d440e32 100644 --- a/requirements/pkg-deps.txt +++ b/requirements/pkg-deps.txt @@ -1,6 +1,6 @@ typing-extensions >=3.10 mypy-extensions >=0.4.3 anyio >=3.0 -jsonpatch >=1.26 +jsonpatch >=1.32 fastjsonschema >=2.14.5 requests >=2.0 diff --git a/requirements/test-env.txt b/requirements/test-env.txt index 6fd1686da..c277b335c 100644 --- a/requirements/test-env.txt +++ b/requirements/test-env.txt @@ -1,8 +1,9 @@ +ipython pytest pytest-asyncio pytest-cov pytest-mock +pytest-rerunfailures pytest-timeout -selenium -ipython responses +selenium diff --git a/setup.cfg b/setup.cfg index 5d1bb1235..b1c314036 100644 --- a/setup.cfg +++ b/setup.cfg @@ -34,6 +34,7 @@ exclude_lines = raise NotImplementedError omit = src/idom/__main__.py + src/idom/core/_fixed_jsonpatch.py [build_sphinx] all-files = true diff --git a/src/idom/core/_fixed_jsonpatch.py b/src/idom/core/_fixed_jsonpatch.py new file mode 100644 index 000000000..702bc4dc6 --- /dev/null +++ b/src/idom/core/_fixed_jsonpatch.py @@ -0,0 +1,56 @@ +# type: ignore + +"""A patched version of jsonpatch + +We need this because of: https://github.com/stefankoegl/python-json-patch/issues/138 + +The core of this patch is in `DiffBuilder._item_removed`. The rest is just boilerplate +that's been copied over with little to no changes. +""" + +from jsonpatch import _ST_REMOVE +from jsonpatch import DiffBuilder as _DiffBuilder +from jsonpatch import JsonPatch as _JsonPatch +from jsonpatch import RemoveOperation, _path_join, basestring +from jsonpointer import JsonPointer + + +def apply_patch(doc, patch, in_place=False, pointer_cls=JsonPointer): + if isinstance(patch, basestring): + patch = JsonPatch.from_string(patch, pointer_cls=pointer_cls) + else: + patch = JsonPatch(patch, pointer_cls=pointer_cls) + return patch.apply(doc, in_place) + + +def make_patch(src, dst, pointer_cls=JsonPointer): + return JsonPatch.from_diff(src, dst, pointer_cls=pointer_cls) + + +class JsonPatch(_JsonPatch): + @classmethod + def from_diff( + cls, + src, + dst, + optimization=True, + dumps=None, + pointer_cls=JsonPointer, + ): + json_dumper = dumps or cls.json_dumper + builder = DiffBuilder(src, dst, json_dumper, pointer_cls=pointer_cls) + builder._compare_values("", None, src, dst) + ops = list(builder.execute()) + return cls(ops, pointer_cls=pointer_cls) + + +class DiffBuilder(_DiffBuilder): + def _item_removed(self, path, key, item): + new_op = RemoveOperation( + { + "op": "remove", + "path": _path_join(path, key), + } + ) + new_index = self.insert(new_op) + self.store_index(item, new_index, _ST_REMOVE) diff --git a/src/idom/core/dispatcher.py b/src/idom/core/dispatcher.py index 5c643abf2..2768879ad 100644 --- a/src/idom/core/dispatcher.py +++ b/src/idom/core/dispatcher.py @@ -19,10 +19,10 @@ from weakref import WeakSet from anyio import create_task_group -from jsonpatch import apply_patch, make_patch from idom.utils import Ref +from ._fixed_jsonpatch import apply_patch, make_patch # type: ignore from .layout import LayoutEvent, LayoutUpdate from .proto import LayoutType, VdomJson diff --git a/src/idom/core/layout.py b/src/idom/core/layout.py index 64f31ac6e..0e228b83f 100644 --- a/src/idom/core/layout.py +++ b/src/idom/core/layout.py @@ -217,10 +217,9 @@ def _render_component( else: key, index = new_state.key, new_state.index if old_state is not None: - assert (key, index) == (old_state.key, old_state.index,), ( + assert key == old_state.key, ( "state mismatch during component update - " - f"key {key!r}!={old_state.key} " - f"or index {index}!={old_state.index}" + f"key {key!r}!={old_state.key!r} " ) parent.children_by_key[key] = new_state # need to do insertion in case where old_state is None and we're appending @@ -538,7 +537,7 @@ def _update_element_model_state( key=old_model_state.key, model=Ref(), # does not copy the model patch_path=old_model_state.patch_path, - children_by_key=old_model_state.children_by_key.copy(), + children_by_key={}, targets_by_event={}, ) diff --git a/src/idom/log.py b/src/idom/log.py index a4c6d6d2b..856ee9ab2 100644 --- a/src/idom/log.py +++ b/src/idom/log.py @@ -1,17 +1,12 @@ import logging import sys from logging.config import dictConfig -from typing import Any from .config import IDOM_DEBUG_MODE -root_logger = logging.getLogger("idom") - - -def logging_config_defaults() -> Any: - """Get default logging configuration""" - return { +dictConfig( + { "version": 1, "disable_existing_loggers": False, "loggers": { @@ -35,10 +30,12 @@ def logging_config_defaults() -> Any: } }, } +) -dictConfig(logging_config_defaults()) +ROOT_LOGGER = logging.getLogger("idom") +"""IDOM's root logger instance""" if IDOM_DEBUG_MODE.current: - root_logger.debug("IDOM is in debug mode") + ROOT_LOGGER.debug("IDOM is in debug mode") diff --git a/src/idom/testing.py b/src/idom/testing.py index 36adc5284..24592208e 100644 --- a/src/idom/testing.py +++ b/src/idom/testing.py @@ -1,13 +1,18 @@ +from __future__ import annotations + import logging import re import shutil +from contextlib import contextmanager from functools import wraps +from traceback import format_exception from types import TracebackType from typing import ( Any, Callable, Dict, Generic, + Iterator, List, Optional, Tuple, @@ -29,6 +34,8 @@ from idom.server.proto import ServerFactory, ServerType from idom.server.utils import find_available_port +from .log import ROOT_LOGGER + __all__ = [ "find_available_port", @@ -166,6 +173,87 @@ def __exit__( return None +@contextmanager +def assert_idom_logged( + match_message: str = "", + error_type: type[Exception] | None = None, + match_error: str = "", + clear_matched_records: bool = False, +) -> Iterator[None]: + """Assert that IDOM produced a log matching the described message or error. + + Args: + match_message: Must match a logged message. + error_type: Checks the type of logged exceptions. + match_error: Must match an error message. + clear_matched_records: Whether to remove logged records that match. + """ + message_pattern = re.compile(match_message) + error_pattern = re.compile(match_error) + + try: + with capture_idom_logs() as handler: + yield None + finally: + found = False + for record in list(handler.records): + if ( + # record message matches + message_pattern.findall(record.getMessage()) + # error type matches + and ( + error_type is None + or ( + record.exc_info is not None + and record.exc_info[0] is not None + and issubclass(record.exc_info[0], error_type) + ) + ) + # error message pattern matches + and ( + not match_error + or ( + record.exc_info is not None + and error_pattern.findall( + "".join(format_exception(*record.exc_info)) + ) + ) + ) + ): + found = True + if clear_matched_records: + handler.records.remove(record) + + if not found: # pragma: no cover + conditions = [] + if match_message: + conditions.append(f"log message pattern {match_message!r}") + if error_type: + conditions.append(f"exception type {error_type}") + if match_error: + conditions.append(f"error message pattern {match_error!r}") + raise AssertionError( + "Could not find a log record matching the given " + + " and ".join(conditions) + ) + + +@contextmanager +def capture_idom_logs() -> Iterator[_LogRecordCaptor]: + """Capture logs from IDOM""" + if _LOG_RECORD_CAPTOR_SINGLTON in ROOT_LOGGER.handlers: + # this is being handled by an outer capture context + yield _LOG_RECORD_CAPTOR_SINGLTON + return None + + ROOT_LOGGER.addHandler(_LOG_RECORD_CAPTOR_SINGLTON) + try: + yield _LOG_RECORD_CAPTOR_SINGLTON + finally: + ROOT_LOGGER.removeHandler(_LOG_RECORD_CAPTOR_SINGLTON) + _LOG_RECORD_CAPTOR_SINGLTON.records = [] + + class _LogRecordCaptor(logging.NullHandler): def __init__(self) -> None: self.records: List[logging.LogRecord] = [] @@ -176,6 +264,9 @@ def handle(self, record: logging.LogRecord) -> bool: return True +_LOG_RECORD_CAPTOR_SINGLTON = _LogRecordCaptor() + + class HookCatcher: """Utility for capturing a LifeCycleHook from a component diff --git a/tests/test_core/test_layout.py b/tests/test_core/test_layout.py index adf307bf6..08df5e7b3 100644 --- a/tests/test_core/test_layout.py +++ b/tests/test_core/test_layout.py @@ -10,10 +10,24 @@ from idom.config import IDOM_DEBUG_MODE from idom.core.dispatcher import render_json_patch from idom.core.layout import LayoutEvent -from idom.testing import HookCatcher, StaticEventHandler +from idom.testing import ( + HookCatcher, + StaticEventHandler, + assert_idom_logged, + capture_idom_logs, +) from tests.general_utils import assert_same_items +@pytest.fixture(autouse=True) +def no_logged_errors(): + with capture_idom_logs() as handler: + yield + for record in handler.records: + if record.exc_info: + raise record.exc_info[1] + + def test_layout_repr(): @idom.component def MyComponent(): @@ -128,25 +142,33 @@ def OkChild(): @idom.component def BadChild(): - raise ValueError("Something went wrong :(") - - with idom.Layout(Main()) as layout: - patch = await render_json_patch(layout) - assert_same_items( - patch.changes, - [ - { - "op": "add", - "path": "/children", - "value": [ - {"tagName": "div", "children": ["hello"]}, - {"tagName": "", "error": "ValueError: Something went wrong :("}, - {"tagName": "div", "children": ["hello"]}, - ], - }, - {"op": "add", "path": "/tagName", "value": "div"}, - ], - ) + raise ValueError("error from bad child") + + with assert_idom_logged( + match_error="error from bad child", + clear_matched_records=True, + ): + + with idom.Layout(Main()) as layout: + patch = await render_json_patch(layout) + assert_same_items( + patch.changes, + [ + { + "op": "add", + "path": "/children", + "value": [ + {"tagName": "div", "children": ["hello"]}, + { + "tagName": "", + "error": "ValueError: error from bad child", + }, + {"tagName": "div", "children": ["hello"]}, + ], + }, + {"op": "add", "path": "/tagName", "value": "div"}, + ], + ) @pytest.mark.skipif( @@ -164,25 +186,30 @@ def OkChild(): @idom.component def BadChild(): - raise ValueError("Something went wrong :(") - - with idom.Layout(Main()) as layout: - patch = await render_json_patch(layout) - assert_same_items( - patch.changes, - [ - { - "op": "add", - "path": "/children", - "value": [ - {"tagName": "div", "children": ["hello"]}, - {"tagName": "", "error": ""}, - {"tagName": "div", "children": ["hello"]}, - ], - }, - {"op": "add", "path": "/tagName", "value": "div"}, - ], - ) + raise ValueError("error from bad child") + + with assert_idom_logged( + match_error="error from bad child", + clear_matched_records=True, + ): + + with idom.Layout(Main()) as layout: + patch = await render_json_patch(layout) + assert_same_items( + patch.changes, + [ + { + "op": "add", + "path": "/children", + "value": [ + {"tagName": "div", "children": ["hello"]}, + {"tagName": "", "error": ""}, + {"tagName": "div", "children": ["hello"]}, + ], + }, + {"op": "add", "path": "/tagName", "value": "div"}, + ], + ) async def test_render_raw_vdom_dict_with_single_component_object_as_children(): @@ -622,11 +649,13 @@ def ComponentReturnsDuplicateKeys(): idom.html.div(key="duplicate"), idom.html.div(key="duplicate") ) - with idom.Layout(ComponentReturnsDuplicateKeys()) as layout: - await layout.render() - - with pytest.raises(ValueError, match=r"Duplicate keys \['duplicate'\] at '/'"): - raise next(iter(caplog.records)).exc_info[1] + with assert_idom_logged( + error_type=ValueError, + match_error=r"Duplicate keys \['duplicate'\] at '/'", + clear_matched_records=True, + ): + with idom.Layout(ComponentReturnsDuplicateKeys()) as layout: + await layout.render() async def test_keyed_components_preserve_hook_on_parent_update(): @@ -652,7 +681,7 @@ def Inner(): assert old_inner_hook is inner_hook.latest -async def test_log_error_on_bad_event_handler(caplog): +async def test_log_error_on_bad_event_handler(): bad_handler = StaticEventHandler() @idom.component @@ -663,14 +692,15 @@ def raise_error(): return idom.html.button({"onClick": raise_error}) - with idom.Layout(ComponentWithBadEventHandler()) as layout: - await layout.render() - event = LayoutEvent(bad_handler.target, []) - await layout.deliver(event) + with assert_idom_logged( + match_error="bad event handler", + clear_matched_records=True, + ): - assert next(iter(caplog.records)).message.startswith( - "Failed to execute event handler" - ) + with idom.Layout(ComponentWithBadEventHandler()) as layout: + await layout.render() + event = LayoutEvent(bad_handler.target, []) + await layout.deliver(event) async def test_schedule_render_from_unmounted_hook(caplog): @@ -689,31 +719,29 @@ def Child(state): idom.hooks.use_effect(lambda: lambda: print("unmount", state)) return idom.html.div(state) - with idom.Layout(Parent()) as layout: - await layout.render() - - old_hook = child_hook.latest + with assert_idom_logged( + r"Did not render component with model state ID .*? - component already unmounted", + ): + with idom.Layout(Parent()) as layout: + await layout.render() - # cause initial child to be unmounted - parent_set_state.current(2) - await layout.render() + old_hook = child_hook.latest - # trigger render for hook that's been unmounted - old_hook.schedule_render() + # cause initial child to be unmounted + parent_set_state.current(2) + await layout.render() - # schedule one more render just to make it so `layout.render()` doesn't hang - # when the scheduled render above gets skipped - parent_set_state.current(3) + # trigger render for hook that's been unmounted + old_hook.schedule_render() - await layout.render() + # schedule one more render just to make it so `layout.render()` doesn't hang + # when the scheduled render above gets skipped + parent_set_state.current(3) - assert re.match( - r"Did not render component with model state ID .*? - component already unmounted", - caplog.records[0].message, - ) + await layout.render() -async def test_layout_element_cannot_become_a_component(caplog): +async def test_layout_element_cannot_become_a_component(): set_child_type = idom.Ref() @idom.component @@ -730,18 +758,21 @@ def Child(): "component": Child(key="the-same-key"), } - with idom.Layout(Root()) as layout: - await layout.render() + with assert_idom_logged( + error_type=ValueError, + match_error="prior element with this key wasn't a component", + clear_matched_records=True, + ): - set_child_type.current("component") + with idom.Layout(Root()) as layout: + await layout.render() - await layout.render() + set_child_type.current("component") - error = caplog.records[0].exc_info[1] - assert "prior element with this key wasn't a component" in str(error) + await layout.render() -async def test_layout_component_cannot_become_an_element(caplog): +async def test_layout_component_cannot_become_an_element(): set_child_type = idom.Ref() @idom.component @@ -758,12 +789,50 @@ def Child(): "component": Child(key="the-same-key"), } - with idom.Layout(Root()) as layout: + with assert_idom_logged( + error_type=ValueError, + match_error="prior element with this key was a component", + clear_matched_records=True, + ): + + with idom.Layout(Root()) as layout: + await layout.render() + + set_child_type.current("element") + + await layout.render() + + +async def test_layout_does_not_copy_element_children_by_key(): + # this is a regression test for a subtle bug: + # https://github.com/idom-team/idom/issues/556 + + set_items = idom.Ref() + + @idom.component + def SomeComponent(): + items, set_items.current = idom.use_state([1, 2, 3]) + return idom.html.div( + [ + idom.html.div( + idom.html.input({"onChange": lambda event: None}), + key=str(i), + ) + for i in items + ] + ) + + with idom.Layout(SomeComponent()) as layout: await layout.render() - set_child_type.current("element") + set_items.current([2, 3]) await layout.render() - error = caplog.records[0].exc_info[1] - assert "prior element with this key was a component" in str(error) + set_items.current([3]) + + await layout.render() + + set_items.current([]) + + await layout.render()