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

Add tests for constructing after destructing #319

Merged
merged 6 commits into from
Mar 17, 2025
Merged

Conversation

jwallwork23
Copy link
Collaborator

Closes #318.

This test extends the fixtures in the existing unit tests for the constructors and destructors to cover the case where torch_tensor_delete is called and then a constructor is applied to the same tensor.

@jwallwork23 jwallwork23 added the testing Related to FTorch testing label Mar 10, 2025
@jwallwork23 jwallwork23 self-assigned this Mar 10, 2025
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Thanks @jwallwork23 Generally looks sensible, couple of queries in there which may just be me not knowing pFUnit as well as you do.

@jwallwork23
Copy link
Collaborator Author

Simplified unit testing in a5db1d6.

Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Thanks @jwallwork23 that is a lot clearer I think.
And I like the improved fixtures use - I had been wondering if something like that was possible actually!

Will approve, only final pedantic point is consider moving test_torch_tensor_delete to the end of the tests rather than between empty and zeros - to me the tests for empty, zeros, and ones should be together. Matter of taste and not going to make it blocking, however.

@jwallwork23
Copy link
Collaborator Author

Will approve, only final pedantic point is consider moving test_torch_tensor_delete to the end of the tests rather than between empty and zeros - to me the tests for empty, zeros, and ones should be together. Matter of taste and not going to make it blocking, however.

Good point. Done in bdcf3cd.

@jatkinson1000
Copy link
Member

Thanks, feel free to merge.

@jwallwork23 jwallwork23 merged commit d3b5099 into main Mar 17, 2025
4 checks passed
@jwallwork23 jwallwork23 deleted the 318_test-destructor branch March 17, 2025 13:36
jwallwork23 added a commit that referenced this pull request Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Related to FTorch testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests for calling destructor and then constructor
2 participants