Unit testing and refactoring of object pool loading #27
Conversation
JannesBrands
requested review from
Thom-de-Jong,
GwnDaan,
ad3154 and
Notgnoshi
and removed request for
Thom-de-Jong,
GwnDaan and
Notgnoshi
| } | ||
|
|
||
| #[test] | ||
| fn test_from_iop() { |
There was a problem hiding this comment.
suggestion: #23 (comment)
My suggestion: unit test the objects directly from a hex string/array. E.g. pass the hex array directly to the de-serialization method for each object individually, and then test it's contents. Then the other way around can be unit tested easily and maintainable as well, by simply serializing the same 'de-serialized' object again and checking if it equals the defined hex array used at the start.
I'm sure we can do a small unit test for the from_iop() that maybe checks the number of objects present in the parsed pool, but doesn't have to check the contents of it. As that should really be done somewhere else in smaller parts IMO.
|
Also I suggest having a look at the failed pipelines, they seems to have some suggestions as well |
|
Yep, completely agree. Keep in mind it's still under work. Forgot to remove that debug stuff. But as you mentioned with injecting the hex into single functions, I'm refactoring right now @Thom-de-Jong "read" function into single object type readings to be able to do it. So stay tuned. This was more or less a progress commit rather than a merge request (still draft). |
Hahah no worries, you requested my review twice so I thought maybe you were in a hurry and wanted some feedback 😄. Other than that sounds good, looking forward to the changes! |
|
Ah lol hahah, yep. I got confused to assign reviewers with #28 . So I was wondering why you are assigned. Later I found out it was this PR and I confused the tabs in chrome and I reassigned you. But thank you for your review! |
| } else { | ||
| NullableObjectId(Some(id)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Im curious for your opinion of this encapsulation to ensure the correctness of attributes that allows or more important, does not allow a null object.
There was a problem hiding this comment.
I like it, though maybe it is simpler to just do:
type NullableObjectId = Option<ObjectId>Then we don't have have to explicitly construct a type and hence it might be easier to use?
GwnDaan
merged commit 5583911
into
Open-Agriculture:tjd/object-pool
|
What the hell, I didn't do that?!? |
Started to create some unit testing for loading iop. I am starting to refactor the Object Pool Reader implementation to assign the unit tests more direct to the different object types. More results will follow soon as I am in vacation.