Skip to content

Conversation

@pranavrajpal
Copy link
Contributor

This adds some run tests to run-singledispatch.test for situations that are likely to be common or that seem like they would be edge cases that we don't handle correctly.

Most of these tests are commented out because mypyc doesn't properly support singledispatch yet, and the few that aren't commented out only work because they're written in a way that causes mypyc to use the standard library implementation of singledispatch.

There are some situations where I added tests that check whether mypyc sticks to what the standard library's implementation does, even though those situations are probably bugs that should probably be mypy errors. For example, testTypeAnnotationsDisagreeWithRegisterArgument checks whether mypyc uses the argument to register for dispatch instead of the type annotation when they disagree, even though having different types for both of those is probably a bug.

Test Plan

I ran all of the run tests that I added and commented out the ones that weren't passing.

Add some run tests to run-singledispatch.test for situations that are
likely to be common or are likely to be edge cases that we don't handle
correctly.

Most of these tests are commented out because mypyc doesn't properly
support singledispatch yet, with the few that aren't commented out
working only because they're written in a way that causes mypyc to use
the standard library implementation of singledispatch.
Copy link
Collaborator

@msullivan msullivan 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 you should be able to add -skip to the test names instead of commenting them out

@msullivan
Copy link
Collaborator

I think you should be able to add -skip to the test names instead of commenting them out

... we ought to add an -xfail sigil that can be put on test names, and register those tests as xfails to pytest; then the tests will be run as part of the test suite, but not fail it, and pytest will report if they unexpectedly pass.

Instead of commenting out run tests for singledispatch that don't pass,
add -skip to their names to make the test runner to make pytest skip
them. That makes commenting them out unnecessary, so this commit also
uncomments all the tests.

These tests should really by xfails, not skips, because we want to know
if these tests suddenly start passing at some time, but the test runner
doesn't support making run tests xfails (yet).
@pranavrajpal
Copy link
Contributor Author

we ought to add an -xfail sigil that can be put on test names, and register those tests as xfails to pytest

I think this is a good idea, but I think skipping these tests will work for now, so I just added -skip to all the previously commented out tests.

I'll create another issue to add xfails to the test runner.

@msullivan
Copy link
Collaborator

These are great, and cover a lot of cases. I'd suggest adding one with a few more classes in it, and some slightly interesting computation. Maybe something like testBenchmarkVisitorTree or testSimulateMypy, but adapted to use singledispatch instead of visitor patterns?

Add 2 run tests for singledispatch that are more complex than the
previous tests and have some more actual computation.
testSingledispatchTreeSumAndEqual is based on testBenchmarkVisitorTree
but uses singledispatch instead of the visitor pattern, and
testSimulateMypySingledispatch is based on mypy's use of singledispatch
in stubtest.py
@pranavrajpal
Copy link
Contributor Author

I added 2 more run tests in 9e0548b. The first one (testSingledispatchTreeSumAndEqual) is meant to be a modified version of testBenchmarkVisitorTree that uses singledispatch and removes the benchmarking (I think https://github.com/mypyc/mypyc-benchmarks would be a better place for benchmarks, right?).

The other test (testSimulateMypySingledispatch) is meant to simulate mypy's use of singledispatch in stubtest.py.

Skip the 2 tests added in the previous commit because they're currently
failing due to lack of singledispatch support. Also removes a yield from
statement that mypy correctly points out has an incorrect type (it tries
to return an iterator of strings, but it should return an iterator of
errors according to the type signature).
Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

Looks good. Switch them to xfail and I'll merge it!

Change the tests that were previously marked as skipped to xfails so
that the tests still get run even if they aren't passing.
@JelleZijlstra JelleZijlstra merged commit 9d92fba into python:master Jun 14, 2021
@pranavrajpal pranavrajpal deleted the add-singledispatch-mypyc-runtests branch June 14, 2021 01:28
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.

3 participants