[Python] Don't rely on possible memory leaks in T(Seq)Collection tests

[guitargeek]

Some unit tests rely on the behavior that when adding object to TLists, Python ownership is always dropped, resulting in memory leaks if the ownership is not correctly handled on the C++ side.

To make these unit tests transparently ready for the moment where the ownership is handled correctly, this commit suggests to use some global lists that keep around Python references until the end of the test. So the effect on the lifetime is the same as having the "controlled" memory leaks.

Spinoff from:

• [Python] Set memory policy to "strict" #13593

[vepadulano]

I have two big doubts about this:

• First, I don't know anymore what these tests are testing. The fact that TList is by default non-owning makes it different from a standard Python list. To the point that I would really wish it was owning by default, and I am partially tempted to just make it so via a Pythonization. But, I am afraid of what could go wrong with such a backwards incompatible change, so I guess we can't really go that way.
• Second, if I suspend my doubts from above and I embrace your suggestion of this PR, I find that there are still many inconsistencies in the way the tests are handled. I have added a couple of comments to express my doubts inline.

[guitargeek]

I have two big doubts about this:

First, I don't know anymore what these tests are testing. The fact that TList is by default non-owning makes it different from a standard Python list. To the point that I would really wish it was owning by default, and I am partially tempted to just make it so via a Pythonization. But, I am afraid of what could go wrong with such a backwards incompatible change, so I guess we can't really go that way.

The tests are testing the Pythonized interface for accessing, adding, and other list mutations, independent of any ownership model. I don't think it matters who owns the contained objects for the sake of this test. Changing the TList behavior to make them owned by default is indeed also too drastic for my taste, and it would also solve the problem entirely. Because the only correct semantics for Python would be shared ownership via Python references. Neither the owning nor the non-owning TList has this semantic.

Second, if I suspend my doubts from above and I embrace your suggestion of this PR, I find that there are still many inconsistencies in the way the tests are handled. I have added a couple of comments to express my doubts inline.

Which inconsistency do you mean? The requirement is that no added TObject should hit zero Python reference count before the end of the tests. That's it's okay to create TObject without adding them to the global list, as long as they are in the same scope as the T(Seq)Collection. The create_tcollection function is the only place where we need to add to the global list, because we need a way to make the objects longer than the function scope.

[vepadulano]

Which inconsistency do you mean? The requirement is that no added TObject should hit zero Python reference count before the end of the tests. That's it's okay to create TObject without adding them to the global list, as long as they are in the same scope as the T(Seq)Collection. The create_tcollection function is the only place where we need to add to the global list, because we need a way to make the objects longer than the function scope.

Alright then I just misinterpreted the tests. It's still weird to see some objects being added to the class-scope list and some others not. But it's not weird because of the way the test is written, the API itself is weird in the first place 👍

[vepadulano]

Thank you for the explanation! I understand better the situation now and I think this can be merged, make sure the CI is green, thanks!

[guitargeek]

Thanks. I'll wait for mac26 and then restart the Windows tests

[github-actions]

Test Results

    22 files      22 suites   3d 5h 18m 51s ⏱️
 3 808 tests  3 807 ✅ 1 💤 0 ❌
76 632 runs  76 623 ✅ 9 💤 0 ❌

Results for commit d61ff57.

♻️ This comment has been updated with latest results.