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

Ensure only public entry points are used in tests #196

Closed
leofang opened this issue Oct 28, 2024 · 6 comments · Fixed by #213
Closed

Ensure only public entry points are used in tests #196

leofang opened this issue Oct 28, 2024 · 6 comments · Fixed by #213
Assignees
Labels
cuda.core Everything related to the cuda.core module P0 High priority - Must do! test Addition or improved tests

Comments

@leofang
Copy link
Member

leofang commented Oct 28, 2024

ex: we shouldn't call Stream._init() in the tests, which is an implementation detail of cuda.core. We should only test public APIs.

It is fine to also have tests for internal modules/functionalities, but they should come after we have coverage for public APIs.

I don't think we have "relational tests" added yet (ex: Device.create_stream() returns a Stream; Stream.record() returns an Event, etc), but let's add them in the next PR.

Originally posted by @leofang in #153 (review)

@leofang leofang added P0 High priority - Must do! test Addition or improved tests cuda.core Everything related to the cuda.core module labels Oct 28, 2024
@leofang leofang added this to the cuda.core beta 2 milestone Oct 28, 2024
@vzhurba01
Copy link
Collaborator

To add to this, each test should be able to run on its own.

Currently some later tests rely on the side-effects of earlier ones such as initializing the Device. Perhaps a fixture would be the right way to resolve this.

@ksimpson-work
Copy link
Contributor

The current implementation is a module level fixture which set_current()'s the default device. However, this could use some work to more robustly test those methods which do require an initialized context. Some form of function level fixture, which avoids the set-once state of Device._has_inited

@leofang
Copy link
Member Author

leofang commented Nov 1, 2024

I was being silly, as far as testing is concerned we can totally do this (untested):

from cuda.core.experimental import _device

@pytest.fixture(scope="module")
def init_cuda():
    device = Device()
    device.set_current()
    yield
    cuda.cuCtxPopCurrent()
    with _device._tls_lock:
        del _device._tls.devices

This ensures:

  • when starting a new test module, we have 1 current context (TLS depth = 1)
  • when leaving the test module, we clean up any cached Device singletons and go back to no active context (TLS depth = 0)

Then there should be no side effects depending on the test order. WDYT?

@vzhurba01
Copy link
Collaborator

That looks good to me. What I would also add is to have each test explicitly state which fixture it uses rather than rely on the implicit.

The use-case this enables is letting us specifying the exact test file to run rather than solely relying on the test filter (i.e. python -m pytest tests/test_module.py -k test_object_code_initialization).

@leofang
Copy link
Member Author

leofang commented Nov 1, 2024

What I would also add is to have each test explicitly state which fixture it uses rather than rely on the implicit.

Yes I think it's totally doable and is a variant of what I suggested earlier. All we need to do is to change the fixture scope from module to function, and pass it explicitly as a test argument to tests that we need explicitness (not all of them need it I think).

@ksimpson-work
Copy link
Contributor

This also looks good to me. I will update the testsuite and make this change + validate it locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda.core Everything related to the cuda.core module P0 High priority - Must do! test Addition or improved tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants