Taichi: [Refactor] Simplify testing parameterization

Created on 11 Jul 2020  路  9Comments  路  Source: taichi-dev/taichi

Concisely describe the motivation
Currently tests often have many layers of decorators. For example,

@ti.require(ti.extension.assertion)
@ti.all_archs_with(debug=True)
@ti.must_throw(RuntimeError)
def test_assert_minimal():
    ti.set_gdb_trigger(False)

    @ti.kernel
    def func():
        assert 0

    func()

In practice, it's really hard to arrange the decorators in the correct order. Actually, the order shown above doesn't work correctly. The current implementation is not easy to debug.

The combination itself is limited. For example, there is all_archs_with(debug=True) but there is no host_arch_with.

Describe the solution you'd like (if any)
It would be good to have a unified ti.test decorator so that the above test can be written as

@ti.test(archs=all, require=ti.extension.assertion, debug=True, gdb_trigger=False, must_throw=RuntimeError)
def test_assert_minimal():    
    @ti.kernel
    def func():
        assert 0

    func()

Additional comments
The current ti.test function should probably be renamed or removed.

This design should be thoroughly discussed before implementation.

discussion feature request

Most helpful comment

@Rullec Please do replacement in these files: tests/python/test_***.py.
Open PR one by one, e.g.: [test] Use @ti.test in test files starting with [a-c].
Since tests are invisible to end-users, we're OK to directly merge these PRs into master, no rullec-test required, WDYT?

Decorator replace rule:

@ti.all_archs  # old API

```py
@ti.test() # new API!

```py
@ti.all_archs_excluding(ti.opengl)  # old API

```py
@ti.test(exclude=ti.opengl) # new API!

or equivalently: @ti.test(exclude=[ti.opengl])

```py
@ti.all_archs    # old API
@ti.require(ti.extension.data64)    # old-fashion multi-decorator design

```py
@ti.test(require=ti.extension.data64) # new API!

```py
@ti.must_throw(RuntimeError)  # don't replace yet, let someone (likely @archibate) take care of them in another PR

Some functions you don't need to replace:

@ti.all_archs
def test_XXX():  # all functions starting with `test_`, please replace my decorator!

@ti.all_archs
def _test_XXX():  # if the name starts with `_test_`, don't replace, let @archibate fix these non-trivial ones

@ti.all_archs
def some_other_even_strange_name_XXX():  # don't replace, let @archibate fix these non-trivial ones

All 9 comments

Agree...

Just FYI, we don't need @ti.must_throw(RuntimeError) at all, the pytest.raises can do that as well:

@ti.require(ti.extension.assertion)
@ti.all_archs_with(debug=True, gdb_trigger=False)
def test_assert_minimal():
    @ti.kernel
    def func():
        assert 0

    with pytest.raises(RuntimeError):
      func()

the pytest.raises can do that as well:

Cool, that works. Thanks. The refactoring is still needed IMHO.

Also FYI: pytest.fixture can do the work of ti.all_archs_with and ti.require as well...
pytest is really a professional package in test, we should learn how to make use of it, instead of writing our ad-hoc decorators.
I believe all the problems we encounter here could be already meet by many other python people and pytest have systematically solutions for them, WDYT :)

In fact, if we make use of pytest.fixture, we can figure out which arch failed test. e.g.:

Before:

python/taichi/misc/test.py::test_this PASSED
python/taichi/misc/test.py::test_that FAILED  # Well... but which arch??

After:

python/taichi/misc/test.py::test_this[x64] PASSED
python/taichi/misc/test.py::test_that[x64] PASSED
python/taichi/misc/test.py::test_this[opengl] PASSED
python/taichi/misc/test.py::test_that[opengl] FAILED  # Oh! It's only OpenGL's fault!

Hi! @Rullec, the infrastructure is quite complete here. It's time to change the @ti.all_archs to @ti.test(...)! Would you like help me? We may use the same workflow as rullec-field did. For documentation about @ti.test(...), see #1475.

Nice, I would be happy to try this one. Thanks for your recommendation.

@Rullec Please do replacement in these files: tests/python/test_***.py.
Open PR one by one, e.g.: [test] Use @ti.test in test files starting with [a-c].
Since tests are invisible to end-users, we're OK to directly merge these PRs into master, no rullec-test required, WDYT?

Decorator replace rule:

@ti.all_archs  # old API

```py
@ti.test() # new API!

```py
@ti.all_archs_excluding(ti.opengl)  # old API

```py
@ti.test(exclude=ti.opengl) # new API!

or equivalently: @ti.test(exclude=[ti.opengl])

```py
@ti.all_archs    # old API
@ti.require(ti.extension.data64)    # old-fashion multi-decorator design

```py
@ti.test(require=ti.extension.data64) # new API!

```py
@ti.must_throw(RuntimeError)  # don't replace yet, let someone (likely @archibate) take care of them in another PR

Some functions you don't need to replace:

@ti.all_archs
def test_XXX():  # all functions starting with `test_`, please replace my decorator!

@ti.all_archs
def _test_XXX():  # if the name starts with `_test_`, don't replace, let @archibate fix these non-trivial ones

@ti.all_archs
def some_other_even_strange_name_XXX():  # don't replace, let @archibate fix these non-trivial ones

Thanks! I have been familiared with this workflow.

Please give me some more time to investigate them carefully and have some PRs!

Since tests are invisible to end-users, we're OK to directly merge these PRs into master, no rullec-test required, WDYT?

We should directly merge into master. It's fine if we use multiple PRs for the replacement.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

yuanming-hu picture yuanming-hu  路  4Comments

jackalcooper picture jackalcooper  路  4Comments

xumingkuan picture xumingkuan  路  3Comments

KLozes picture KLozes  路  4Comments

quadpixels picture quadpixels  路  3Comments