[red-knot] Unify setup_db() functions, add TestDb builder#14777
[red-knot] Unify setup_db() functions, add TestDb builder#14777
setup_db() functions, add TestDb builder#14777Conversation
| let db = TestDbBuilder::new() | ||
| .with_python_version(PythonVersion::PY313) | ||
| .build()?; |
There was a problem hiding this comment.
This is the only functional change, without any consequences. But it feels okay, since typing.NoDefault is only available since 3.13). I will need this — and similar things — soon in the statically-know branch, so I'm putting it into place already.
There was a problem hiding this comment.
I think once we have the statically-known branch, we will probably want to add more tests around some of these "exists in typing in recent versions and in typing_extensions as fallback" known symbols, in terms of how they behave on older and newer Pythons. But this change seems fine for this PR and this particular test.
|
carljm
left a comment
There was a problem hiding this comment.
Thank you! Much nicer. This always bugged me slightly but never enough to fix it :)
| let db = TestDbBuilder::new() | ||
| .with_python_version(PythonVersion::PY313) | ||
| .build()?; |
There was a problem hiding this comment.
I think once we have the statically-known branch, we will probably want to add more tests around some of these "exists in typing in recent versions and in typing_extensions as fallback" known symbols, in terms of how they behave on older and newer Pythons. But this change seems fine for this PR and this particular test.
| #[test] | ||
| fn typing_vs_typeshed_no_default() { | ||
| let db = setup_db(); | ||
| fn typing_vs_typeshed_no_default() -> anyhow::Result<()> { |
There was a problem hiding this comment.
@AlexWaygood previously expressed a preference for using unwrap() in tests, rather than having tests return a Result and then using ? in the test. We have some existing tests using both styles. I don't have strong feelings, but I think I shared Alex's preference after writing tests in both styles; the need to end the test with Ok(()) is irritating.
Do you have a preference in the other direction?
There was a problem hiding this comment.
Do you have a preference in the other direction?
I do not. I usually also use .unwrap() liberally in tests. Sometimes the code looks a lot nicer if there are many .unwrap()s that can be replaced with ?, so I guess it's okay to not have a strict guideline(?).
For TestDbBuilder specifically, I first wrote a version that didn't return a Result. I then changed it because I thought there might be more complex test setups in the future where it would be useful for the developer to see a good error message if they did something wrong in the setup of the test.
And note that setup_db() still returns a plain TestDb, not a Result. It's just the builder that returns a Result.
For the particular example here... nothing can go wrong when just setting the Python version, so I'll change this back to .unwrap() style.
Summary
setup_dbfunctions, use just one in a single central place.TestDbBuilderthat can control the Python target version, custom typeshed, and pre-existing files.The main motivation for this is that we're soon going to need customization of the Python version, and I didn't feel like adding this to each of the existing
setup_dbfunctions.It feels like there is much more we can do, especially around writing files, but let's first see if everyone is happy with this first step.