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

Allow app-dir parameter on the run() function #1271

Merged
merged 7 commits into from
Dec 6, 2021

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Nov 27, 2021

Closes #1126

@Kludex
Copy link
Sponsor Member Author

Kludex commented Nov 27, 2021

@HansBrende You recommended the solution, so feel free to review as well. 🙏

@Kludex Kludex mentioned this pull request Nov 27, 2021
4 tasks
@Kludex Kludex requested a review from euri10 November 27, 2021 19:08
Copy link
Contributor

@HansBrende HansBrende left a comment

Choose a reason for hiding this comment

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

LGTM!

@Kludex
Copy link
Sponsor Member Author

Kludex commented Dec 4, 2021

@euri10 ping

Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

I think this looks good, the only small thing that may make it better is adding a test, I realize original PR that introduced the feature doesn't have one either so we're naked here and this will hit when working again on coverage anyway.
So approved, but feel free to add tests ;)

@Kludex
Copy link
Sponsor Member Author

Kludex commented Dec 6, 2021

I will add, thanks. ✌️

@Kludex
Copy link
Sponsor Member Author

Kludex commented Dec 6, 2021

How would you test this?

I started with this thought:

def test_app_dir(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None:
    app_dir = tmp_path / "dir" / "app_dir"
    app_file = app_dir / "main.py"
    app_dir.mkdir(parents=True)
    app_file.touch()
    app_file.write_text(
        dedent(
            """
            async def app(scope, receive, send):
                ...
            """
        )
    )
    run("main:app", app_dir=str(app_dir))

But the server will run, and we don't want that. We use run_server in similar cases.

After that I thought: "we'll, let's mock the server.run. But if I do that, we'll not test that we reached the import_string successfully...

Ideas?

@euri10 euri10 mentioned this pull request Dec 6, 2021
@humrochagf
Copy link
Contributor

Hi!

You can try to use the way I did for the cli tools https://github.com/encode/uvicorn/blob/master/tests/test_cli.py#L69-L78
Mock just the Config.bind_socket and Server.run to prevent the actual server from running and also from locking the socket.

Then you can check sys.path to validate the app_dir path was added to it

@Kludex
Copy link
Sponsor Member Author

Kludex commented Dec 6, 2021

I'm going to add the test suggested by @humrochagf (thanks).

But that's not fully what I want. I want to be able to know that app was imported. But to do that, I'll have to complicate things.

@Kludex Kludex merged commit edb6454 into encode:master Dec 6, 2021
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* Allow app-dir parameter on the run() function

* Add default value to pop() call

* Update GitHub templates with new forms

* Add test
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.

allow uvicorn.run to also accept app-dir kwarg
4 participants