Skip to content

added the missing tests (and verified they pass) #6170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
patrizzzz wants to merge 1 commit into
base: main
Choose a base branch
from
+61 −10
Open

added the missing tests (and verified they pass) #6170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 37 additions & 5 deletions tests/units/compiler/test_compiler_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,39 @@
def TestState(State):
pass
from __future__ import annotations

import asyncio

def test_compile_state():
# TODO: Implement test for compile_state function.
pass
import pytest

from reflex.compiler.utils import compile_state
from reflex.constants.state import FIELD_MARKER
from reflex.state import State
from reflex.vars.base import computed_var


class CompileStateState(State):
a: int = 1
b: int = 2

@computed_var
async def async_value(self) -> str:
await asyncio.sleep(0)
return "resolved"


def _get_state_values(compiled: dict, state: type[State]) -> dict:
return compiled[state.get_full_name()]


def test_compile_state_resolves_async_computed_vars_without_event_loop():
compiled = compile_state(CompileStateState)
values = _get_state_values(compiled, CompileStateState)
assert values[f"a{FIELD_MARKER}"] == 1
assert values[f"b{FIELD_MARKER}"] == 2
assert values[f"async_value{FIELD_MARKER}"] == "resolved"


@pytest.mark.asyncio
async def test_compile_state_resolves_async_computed_vars_with_running_event_loop():
compiled = compile_state(CompileStateState)
values = _get_state_values(compiled, CompileStateState)
assert values[f"async_value{FIELD_MARKER}"] == "resolved"
Comment on lines +35 to +39
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing assertions for regular state vars in async-loop test

The synchronous counterpart (test_compile_state_resolves_async_computed_vars_without_event_loop) asserts that both regular state vars (a, b) and the async computed var are resolved correctly. This test only checks async_value, leaving the regular-var assertions untested for the running-event-loop code path. Consider adding the same assertions for completeness and consistency:

Suggested change
@pytest.mark.asyncio
async def test_compile_state_resolves_async_computed_vars_with_running_event_loop():
compiled = compile_state(CompileStateState)
values = _get_state_values(compiled, CompileStateState)
assert values[f"async_value{FIELD_MARKER}"] == "resolved"
@pytest.mark.asyncio
async def test_compile_state_resolves_async_computed_vars_with_running_event_loop():
compiled = compile_state(CompileStateState)
values = _get_state_values(compiled, CompileStateState)
assert values[f"a{FIELD_MARKER}"] == 1
assert values[f"b{FIELD_MARKER}"] == 2
assert values[f"async_value{FIELD_MARKER}"] == "resolved"

29 changes: 24 additions & 5 deletions tests/units/test_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1670,13 +1670,32 @@ def test_call_app():
assert isinstance(api, Starlette)


def test_app_with_optional_endpoints():
def test_app_with_optional_endpoints(tmp_path: Path):
from reflex.components.core.upload import Upload

app = App()
Upload.is_used = True
app._add_optional_endpoints()
# TODO: verify the availability of the endpoints in app.api
from starlette.routing import Mount, Route

Upload.is_used = False
with chdir(tmp_path):
app = App()
Upload.is_used = True
app._add_optional_endpoints()

assert app._api is not None
upload_path = str(constants.Endpoint.UPLOAD)
routes = list(app._api.routes)

assert any(
isinstance(r, Route)
and r.path == upload_path
and "POST" in getattr(r, "methods", set())
for r in routes
)
assert any(
isinstance(r, Mount) and r.path == upload_path and r.name == "uploaded_files"
for r in routes
)
Upload.is_used = False
Comment on lines +1697 to +1698
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragile cleanup – Upload.is_used not reset on test failure

Upload.is_used = False is placed after the with chdir(tmp_path): block without a try/finally guard. If either assert any(...) call on lines 1688–1696 raises an AssertionError, the cleanup line never executes and Upload.is_used is left as True for subsequent tests. Wrap the assertions in try/finally or, preferably, use the existing monkeypatch fixture for test-scoped teardown:

Suggested change
)
Upload.is_used = False
finally:
Upload.is_used = False

Or, restructure the test to use the monkeypatch fixture:

def test_app_with_optional_endpoints(tmp_path: Path, monkeypatch):
    from reflex.components.core.upload import Upload
    from starlette.routing import Mount, Route

    monkeypatch.setattr(Upload, "is_used", False)
    with chdir(tmp_path):
        app = App()
        monkeypatch.setattr(Upload, "is_used", True)
        app._add_optional_endpoints()
        ...

Using monkeypatch guarantees restoration to the original value regardless of how the test exits.

Copy link
Collaborator

@masenf masenf Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggest setting Upload.is_used with monkeypatch inside a fixture that this test depends on, that way the clean up and test logic are cleanly separated

Copy link
Contributor

@greptile-apps greptile-apps bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call – that's cleaner. Here's how it would look:

Suggested change
)
Upload.is_used = False
@pytest.fixture
def upload_component(monkeypatch):
from reflex.components.core.upload import Upload
monkeypatch.setattr(Upload, "is_used", False)
yield Upload
def test_app_with_optional_endpoints(tmp_path: Path, upload_component):
from starlette.routing import Mount, Route
with chdir(tmp_path):
app = App()
upload_component.is_used = True
app._add_optional_endpoints()
assert app._api is not None
upload_path = str(constants.Endpoint.UPLOAD)
routes = list(app._api.routes)
assert any(
isinstance(r, Route)
and r.path == upload_path
and "POST" in getattr(r, "methods", set())
for r in routes
)
assert any(
isinstance(r, Mount) and r.path == upload_path and r.name == "uploaded_files"
for r in routes
)

The fixture ensures Upload.is_used is False initially and handles cleanup automatically, while the test logic stays focused on the actual behavior being tested.

Copy link
Contributor

@greptile-apps greptile-apps bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I can't provide this as an inline suggestion because:

  1. The fixture needs to be placed before the test function
  2. The comment is only on lines 1697-1698 (the function signature)
  3. Inline suggestions only modify the exact lines the comment is attached to

Here's the pattern you should use:

@pytest.fixture
def upload_enabled(monkeypatch):
    """Fixture that enables Upload and cleans up afterward."""
    from reflex.components.core.upload import Upload
    
    monkeypatch.setattr(Upload, "is_used", True)
    yield
    monkeypatch.setattr(Upload, "is_used", False)


def test_app_with_optional_endpoints(tmp_path: Path, upload_enabled):
    from reflex.components.core.upload import Upload
    from starlette.routing import Mount, Route

    with chdir(tmp_path):
        app = App()
        app._add_optional_endpoints()

        assert app._api is not None
        upload_path = str(constants.Endpoint.UPLOAD)
        routes = list(app._api.routes)

        assert any(
            isinstance(r, Route)
            and r.path == upload_path
            and "POST" in getattr(r, "methods", set())
            for r in routes
        )
        assert any(
            isinstance(r, Mount) and r.path == upload_path and r.name == "uploaded_files"
            for r in routes
        )

Place the upload_enabled fixture above the test function. The fixture handles both setup and teardown automatically, keeping the test logic clean.



def test_app_state_manager():
Expand Down