Complete serde unit tests#2762
Complete serde unit tests#2762iamtrask merged 34 commits intoOpenMined:masterfrom vvmnnnkv:refactor-serde
Conversation
|
Wow. I'm so proud of you @vvmnnnkv. This is one hell of a PR, nicely done. Let's get this reviewed and merged! |
|
Indeed! Quite an epic PR! Well done sir! Looks like there are two Travis errors for now. One is a failing unit test for "serde coverage" and another is a failing style test because some part of our unit tests aren't getting run (likely because it . fails before it finishes) |
|
|
||
|
|
||
| ######################################################################## | ||
| # Functions that return list of serde samples in the following format: |
There was a problem hiding this comment.
Since these tests also function as documentation for consumers (like the JS and Android workers), they would be a little easier to read if the samples were dictionaries. Readers wouldn't have to keep the format in mind from this comment at the top. Might also be helpful in the case where a change breaks one or two tests and someone navigates directly to them without reading the whole file.
There was a problem hiding this comment.
I agree on this point, but more because of test_serde_simplify which is not 100% clear because it uses indices of the tuple.
By the way, I see that when you define a compare function, you always do for 1 (custom_detailed_values_comparison_function ?) and the other one (custom_simplified_values_comparison_function ?) is always None, is this last one needed?
There was a problem hiding this comment.
Thanks, that helps! Turns out I was misreading the examples the first time. 😅
LaRiffle
left a comment
LaRiffle
left a comment
There was a problem hiding this comment.
That's amazing.
And so clean :O
|
|
||
| def test_serde_coverage(): | ||
| """Checks all types in serde are tested""" | ||
| for cls, _ in serde.simplifiers.items(): |
|
Fixed old problems, but now need to add new PrivateTensor and PromiseTensor to pass tests 😅 |
|
Looks good to me. Let's get this merged @LaRiffle @iamtrask @karlhigley |
|
Looks like there are lines in test_serde_full.py which aren't getting run - this is causing Travis to fail. |
|
These were lines of code converted to torchscript (e.g. via @torch.jit.script or torch.jit.ScriptModule). They are not executed, so just skipped them from report with |
5 participants
This is for issue #2654
PR tries to fully cover serde with unit tests. As a byproduct it also kind of documents how serde simplifies values (by providing reference simplification samples) and reveals some inconsistencies in how simplification is done.
New
test/test_serde_full.pycontains 3 generic tests:samplesdict that contains samples for each serde type is supplied to these tests.Additionally, there're few fixes here to make tests pass:
I had to exclude this file from black formatting because it squashes reference simplification examples and makes them less clear.