The Wayback Machine - https://web.archive.org/web/20220411225515/https://github.com/RustPython/RustPython/issues/3348
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

[RFC] Consolidating RustPython's annotations in the CPython test suite from interleaved annotations to single blocks #3348

Open
fanninpm opened this issue Oct 19, 2021 · 0 comments
Labels

Comments

@fanninpm
Copy link
Contributor

@fanninpm fanninpm commented Oct 19, 2021

Summary

Currently, tests in Lib/test that are erroring/failing are marked as failures or skips in various ways. I propose that, for each test_*.py file in Lib/test/**, we collect most (if not all) instances of skipped tests into a single section at the bottom of the test file.

Detailed Explanation and Rationale

Often, when someone adds a CPython test to Lib/test, function decorations from unittest will have to be added in order for the test suite to pass muster. If a test function should fail, it should get marked as an expected failure so the CI can flag it once it passes. However, I have found several problems with this approach, based on my observations:

  1. Unexpected passes are often hard to find within a large file of tests. (Line numbers for unexpected passes are not reported when the suite is invoked with -v, and function names can overlap.)
  2. When a test fails under certain test cases but not others, I have often placed a four-liner within the offending test case/class (the last four lines in the example below). This four-liner can be difficult for newcomers to intuit, especially when the test unexpectedly passes, and the construct has to be dismantled.
class TestWhatever(CommonTests, unittest.TestCase):
    ...
    # TODO: RUSTPYTHON
    @unittest.expectedFailure
    def test_function(self):
        super().test_function()
  1. When a test fails on a certain OS (or only on GitHub Actions), I have often placed a three/four-liner within the offending test case/class, but outside the test function (see examples below). Like the example above, this construct can be difficult for newcomers to intuit, especially when the test unexpectedly passes, and the construct has to be dismantled.
# TODO: RUSTPYTHON
import os, sys # depends on what has already been imported at the top of the test file
if sys.platform == "win32" and os.getenv("CI"):
    test_windows_only_failure = unittest.expectedFailure(test_windows_only_failure)
  1. When a test is not explicitly defined as a function, I would have to explicitly mark it as a test failure without using the annotation. Newer contributors may skip the test case outright.
  2. Interspersing our annotations within the CPython test suite can make it difficult to upgrade the test suite to newer versions of CPython.

I propose adding a block near the bottom of every CPython test file that contains most (if not all) of the RustPython-specific test annotations. The block in question would be delineated by comments and would appear before the invocation of main() or unittest.main(), as shown:

...
### TODO: RUSTPYTHON, BEGIN TEST ANNOTATION BLOCK
import os, sys, unittest
CPythonTestCase.test_failing_function = unittest.expectedFailure(CPythonTestCase.test_failing_function)
CPythonTestCase.test_fails_only_sometimes = unittest.expectedFailure(lambda *args, **kwargs: CPythonTestCase.mro()[1].fails_only_sometimes(*args, **kwargs))
CPythonTestCase.test_panic = unittest.skip(CPythonTestCase.test_panic, "TODO: RUSTPYTHON, thread 'main' panicked at 'panic!()'")
if sys.platform == "win32" and os.getenv("CI"):
    CPythonTestCase.test_fails_on_windows = unittest.expectedFailure(CPythonTestCase.test_fails_on_windows)
### TODO: RUSTPYTHON, END TEST ANNOTATION BLOCK
if __name__ == "__main__":
    unittest.main()

(In the tes_fails_only_sometimes example, lambda is used to create a completely new function, so the call to unittest.expectedFailure() does not affect the other cases in which this function passes. The mro()[1] indirection is to prevent a recursion error (although the indexing could be improved).)
This would also confer the benefit of making it easier for someone to (semi-)automate marking a test file from CPython. A tool could run a test suite, find the failing tests (if the suite doesn't cause RustPython to panic), and add them to the block. Once a test unexpectedly passes, the relevant annotation can easily be removed from the block.

Drawbacks and Alternatives

I see a couple of drawbacks with this solution:

  1. Not everything can fit "neatly" within this block. Sometimes, manually editing a test file is unavoidable because that could be the only way to make sure the test file can get parsed without errors. (See, for example, these lines in test_time.py.)
  2. Converting our current suite from inline annotations to the annotation block is a mammoth endeavor, and the efforts may not pay off immediately.

The obvious alternative is to do nothing, as the status quo is working well enough.

Unresolved Questions

  • Should the block have a specified format?
  • Are there any existing tools that can modify python files in this or a similar way?
  • Suggestions are welcome!
@fanninpm fanninpm added the RFC label Oct 19, 2021
@fanninpm fanninpm changed the title [RFC] Consolidating RustPython's annotations in tjhe CPython test suite [RFC] Consolidating RustPython's annotations in the CPython test suite from interleaved annotations to single blocks Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant