Skip to content
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.

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

tests: Add skipped coverages for websockets.py and templating.py using branch=true #2793

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

lealre
Copy link
Contributor

@lealre lealre commented Dec 12, 2024

Note ℹ️
I don't have a lot of experience, and the idea here is to try to help. Sorry if something seems silly or obvious. Any feedback on how I can improve or suggestions for best practices would be appreciated!

Summary

Regarding the issue #2452

Changes to starlette/websockets.py

This test is designed to skip the if statement at line 106. Specifically, the client_state is set to CONNECTED before calling websocket.accept(), which ensures that the code bypasses the conditional logic meant for the CONNECTING state.

I replicated the first test and mocked the value.

Changes to starlette/templating.py

At line 97, an assertion ensures that either directory or env must be provided, but not both None. Since it's not possible to skip both conditionals (if or elif), I added a # pragma: no cover comment to prevent code coverage from flagging this as untested.

def __init__(
    self,
    directory: str | PathLike[str] | typing.Sequence[str | PathLike[str]] | None = None,
    *,
    context_processors: list[typing.Callable[[Request], dict[str, typing.Any]]] | None = None,
    env: jinja2.Environment | None = None,
    **env_options: typing.Any,
) -> None:
    if env_options:
        warnings.warn(
            "Extra environment options are deprecated. Use a preconfigured jinja2.Environment instead.",
            DeprecationWarning,
        )
    assert jinja2 is not None, "jinja2 must be installed to use Jinja2Templates"  
    assert bool(directory) ^ bool(env), "either 'directory' or 'env' arguments must be passed" # <--------------- Line 97
    self.context_processors = context_processors or []
    if directory is not None:   # <-------------------------------------------- Line 99
        self.env = self._create_env(directory, **env_options)
    elif env is not None:       # <-------------------------------------------- Line 101
        self.env = env

    self._setup_env_defaults(self.env)

EDIT: Another option could be to separate the conditional blocks as shown below, ensuring that all branches are covered and that directory takes precedence over env if both are provided, as the current code behaves.

Regarding the precedence of the directory over env, would it be worth adding this to the documentation?

    def __init__(
        self,
        directory: str | PathLike[str] | typing.Sequence[str | PathLike[str]] | None = None,
        *,
        context_processors: list[typing.Callable[[Request], dict[str, typing.Any]]] | None = None,
        env: jinja2.Environment | None = None,
        **env_options: typing.Any,
    ) -> None:
        if env_options:
            warnings.warn(
                "Extra environment options are deprecated. Use a preconfigured jinja2.Environment instead.",
                DeprecationWarning,
            )
        assert jinja2 is not None, "jinja2 must be installed to use Jinja2Templates"
        assert bool(directory) ^ bool(env), "either 'directory' or 'env' arguments must be passed"
        self.context_processors = context_processors or []
        if env is not None:
            self.env = env
        if directory is not None:
            self.env = self._create_env(directory, **env_options)

        self._setup_env_defaults(self.env)

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@Kludex
Copy link
Member

Kludex commented Dec 13, 2024

Note ℹ️
I don't have a lot of experience, and the idea here is to try to help. Sorry if something seems silly or obvious. Any feedback on how I can improve or suggestions for best practices would be appreciated!

It's all good. :)

Thanks for the help.

Comment on lines 616 to 627
def test_websocket_accept_with_mocked_connected_state(test_client_factory: TestClientFactory) -> None:
async def app(scope: Scope, receive: Receive, send: Send) -> None:
websocket = WebSocket(scope, receive=receive, send=send)
websocket.client_state = WebSocketState.CONNECTED
await websocket.accept()
await websocket.send_json({"url": str(websocket.url)})
await websocket.close()

client = test_client_factory(app)
with client.websocket_connect("/123?a=abc") as websocket:
data = websocket.receive_json()
assert data == {"url": "ws://testserver/123?a=abc"}
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to have tests that the only way to reproduce the path is with mocks.

The right way I believe would be to remove the conditional in this case, but I actually think it helps on the understanding of the states there. Can we just have a pragma: no branch there?

Copy link
Member

Choose a reason for hiding this comment

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

no branch, not no cover 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that

@@ -99,7 +99,7 @@ def __init__(
if directory is not None:
self.env = self._create_env(directory, **env_options)
elif env is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this instead?

Suggested change
elif env is not None:
elif env is not None: # pragma: no branch

@Kludex
Copy link
Member

Kludex commented Dec 13, 2024

Vlw. :)

@Kludex Kludex enabled auto-merge (squash) December 13, 2024 22:42
@Kludex Kludex merged commit 5000c9f into encode:master Dec 13, 2024
6 checks passed
@lealre lealre deleted the test-branch-coverage branch December 13, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants