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

Cut down unit test code using destructor #298

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

jwallwork23
Copy link
Collaborator

This PR tidies up the unit tests considerably in light of #297.

There's no longer any need to call torch_tensor_delete (except for the test of it). I also spotted a way to refactor the error raising so that we don't need all the clean_up subroutines.

In some cases, we can use the inbuilt pFUnit assertions, which is nice.

@jwallwork23 jwallwork23 added the testing Related to FTorch testing label Feb 26, 2025
@jwallwork23 jwallwork23 self-assigned this Feb 26, 2025
@jwallwork23 jwallwork23 marked this pull request as ready for review February 27, 2025 08:47
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.

Yes, all makes sense to me. Thanks @jwallwork23
I like that the nullification is now at the same level as declaration of tensors/pointers and agree we should use the pFUnit preprocessor directives where we can.

Couple of questions around @assertEqual:

  • What is the output of @assertEqual? Previously you had very explicit error messages relating to the test/routine/rank whereas I presume they are now more opaque/generic?
  • I presume you checked that @assertEqual fails as expected when you break something? 😉

@jwallwork23
Copy link
Collaborator Author

* What is the output of `@assertEqual`? Previously you had very explicit error messages relating to the test/routine/rank whereas I presume they are now more opaque/generic?

So with the diff

diff --git a/test/unit/test_tensor_constructors.pf b/test/unit/test_tensor_constructors.pf
index 5e0e2c8..3b7d94a 100644
--- a/test/unit/test_tensor_constructors.pf
+++ b/test/unit/test_tensor_constructors.pf
@@ -93,7 +93,7 @@ contains
     call torch_tensor_delete(tensor)
 
     ! Check torch_tensor_delete does indeed free the memory
-    @assertFalse(c_associated(tensor%p))
+    @assertTrue(c_associated(tensor%p))
 
   end subroutine test_torch_tensor_empty

we get the failure

1: Failure
1:  in: 
1: test_tensor_constructors_suite.test_torch_tensor_empty[F][F]
1:   Location: 
1: [test_tensor_constructors.pf:96]
1: 
1:   
1: Failure
1:  in: 
1: test_tensor_constructors_suite.test_torch_tensor_empty[T][T]
1:   Location: 
1: [test_tensor_constructors.pf:96]

Personally, I think the line number is sufficient for reports of failed assertions.

* I presume you checked that `@assertEqual` fails as expected when you break something? 😉

Yep

@jwallwork23 jwallwork23 merged commit fe43881 into 295_oo-mem-leak Feb 27, 2025
5 checks passed
@jwallwork23 jwallwork23 deleted the 295_auto-delete branch February 27, 2025 10:30
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.

2 participants