Skip to content
New issue

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

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

Already on GitHub? # to your account

[red-knot] consolidate diagnostic and inference tests #13248

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Sep 4, 2024

Pull the tests from types.rs into infer.rs.

All of these are integration tests with the same basic form: create a code sample, run type inference or check on it, and make some assertions about types and/or diagnostics. These are the sort of tests we will want to move into a test framework with a low-boilerplate custom textual format. In the meantime, having them together (and more importantly, their helper utilities together) means that it's easy to keep tests for related language features together (iterable tests with other iterable tests, callable tests with other callable tests), without an artificial split based on tests which test diagnostics vs tests which test inference. And it allows a single test to more easily test both diagnostics and inference. (Ultimately in the test framework, they will likely all test diagnostics, just in some cases the diagnostics will come from reveal_type().)

@carljm carljm added the red-knot Multi-file analysis & type inference label Sep 4, 2024
Copy link
Contributor

github-actions bot commented Sep 4, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I would be more leaning towards moving the tests in the other direction, considering that types is the API but whatever. Let's build a testing framework instead to solve this for good.

Base automatically changed from cjm/annassign-no-rhs to main September 5, 2024 15:55
@carljm
Copy link
Contributor Author

carljm commented Sep 5, 2024

Yeah we could just as well put them in types.rs. It's not hard to do, I can push that change. But I agree, hopefully they don't stay in either of these files for too much longer.

@carljm
Copy link
Contributor Author

carljm commented Sep 5, 2024

Actually I'm not going to push that change because it will cause too much pain in rebasing my WIP on declared types, where I add a bunch of new tests :) We'll just leave them in infer.rs for now until they get moved into a testing framework.

@MichaReiser
Copy link
Member

Yeah we could just as well put them in types.rs. It's not hard to do, I can push that change. But I agree, hopefully they don't stay in either of these files for too much longer.

I'm fine either way. I prefer what ever takes the least of your time (which is what you have now)

@carljm carljm merged commit a4ebe7d into main Sep 5, 2024
20 checks passed
@carljm carljm deleted the cjm/tests-together branch September 5, 2024 16:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants