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

Increased test coverage for serialization #311

Merged
merged 4 commits into from
Jul 17, 2018

Conversation

lumip
Copy link
Contributor

@lumip lumip commented Jul 13, 2018

Closes #294

I looked into coveralls to see which lines/branches are not yet covered by tests.
In particular the case in Serializable.init() where a pulse might get registered only after the gc is invoked was not covered. I tried to do this (delete a registered pulse with del while explicitely disabling the automatic gc) but couldn't cause the desired behavior. Are we sure that the invocation of the gc is necessary at that point?
Also, there's the issue of the AnonymousSerializable. It is currently not neatly integrated into the Serializable class tree and should really be (or be an abstract class defining abstract methods).

@lumip lumip added this to the New Serialization Routines milestone Jul 13, 2018
@lumip lumip requested a review from terrorfisch July 13, 2018 14:17
@coveralls
Copy link

coveralls commented Jul 13, 2018

Coverage Status

Coverage increased (+0.07%) to 94.147% when pulling 0d1f9df on issues/250_one_more_test into 42b888d on issues/250_serialization.

@terrorfisch
Copy link
Member

terrorfisch commented Jul 14, 2018

Are we sure that the invocation of the gc is necessary at that point?

I think it can be removed as the default_storage_backend isn't a WeakValueDictionary by default anymore.

Also, there's the issue of the AnonymousSerializable. It is currently not neatly integrated into the Serializable class tree and should really be (or be an abstract class defining abstract methods).

An ABCMeta metaclass makes calling isinstance much more costly(~10) and we call isinstance with Expression as type quite a lot. We could drop this and just check if the object we want to serialize has a get_serialization_data attribute here: https://github.com/qutech/qc-toolkit/blob/9548742bb565ce50171eacfd8e47eff02df6b0d8/qctoolkit/serialization.py#L861

@lumip
Copy link
Contributor Author

lumip commented Jul 16, 2018

I think it can be removed as the default_storage_backend isn't a WeakValueDictionary by default anymore.

hm.. but in principle it's still possible to set the default_storage_backend as a WeakValueDictionary so if the gc invocation is necessary in that case we shouldn't just remove that (although the tests I did worked fine without the gc even for WeakValueDictionary but that might be luck?)... However, the weakref documentation states that

if the referent is no longer alive, calling the reference object will cause None to be returned

so I guess, instead of invoking the garbace collector, we can just check whether the object currently stored in the registry (if any) is None ..?

An ABCMeta metaclass makes calling isinstance much more costly(~10) and we call isinstance with Expression as type quite a lot. We could drop this and just check if the object we want to serialize has a get_serialization_data attribute here:

-.-
well.. in that case I guess it's best to leave it as it is right now (and maybe try to find a nicer solution for the Expressions in the long run..?)

@terrorfisch
Copy link
Member

hm.. but in principle it's still possible to set the default_storage_backend as a WeakValueDictionary so if the gc invocation is necessary in that case we shouldn't just remove that (although the tests I did worked fine without the gc even for WeakValueDictionary but that might be luck?)... However, the weakref documentation states that

IMHO, if the user sets it as an WeakValueDict, he is responsible for cleaning up. If desired we could provide tools for this.

@terrorfisch
Copy link
Member

Merge if you think this is ok.

@lumip lumip merged commit 47338ee into issues/250_serialization Jul 17, 2018
@lumip lumip deleted the issues/250_one_more_test branch July 19, 2018 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants