Mocking utils test - #107
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #107 +/- ##
==========================================
+ Coverage 63.57% 70.92% +7.34%
==========================================
Files 12 13 +1
Lines 2570 2879 +309
==========================================
+ Hits 1634 2042 +408
+ Misses 936 837 -99
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: frheault <10820351+frheault@users.noreply.github.com>
Co-authored-by: frheault <10820351+frheault@users.noreply.github.com>
Co-authored-by: frheault <10820351+frheault@users.noreply.github.com>
|
@arokem I finally came back to this ! (and updated with master) Could be useful to have this before moving out the materials to TRX-UTILS side repo. |
|
Yes, agreed. Is this still WIP or do you want me to review / merge? |
|
Ready for review ! |
arokem
left a comment
arokem
left a comment
There was a problem hiding this comment.
Looks good! I spotted some smoketests, which could be made stronger, if you feel that's merited. Also, could use some explanation about the changes in workflows.py.
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| tmp_name = os.path.join(tmpdir, "test.mmap") | ||
| mmap_arr = np.memmap(tmp_name, dtype="float32", mode="w+", shape=(10,)) | ||
| close_or_delete_mmap(mmap_arr) |
There was a problem hiding this comment.
This is just a smoke test for now. Do you want to assert anything about the file after this operation/
| seq._offsets = offsets | ||
| seq._lengths = np.array([2, 2, 2, 2, 2], dtype="uint32") | ||
|
|
||
| close_or_delete_mmap(seq) |
There was a problem hiding this comment.
Same comment here: for now, this is just a smoke test, so maybe add an assertion on properties of the mmap/file.
| dtype=dict_dtype[key][key_dpv], | ||
| mode="w+", | ||
| shape=trx.data_per_vertex[key_dpv]._data.shape, | ||
| with get_trx_tmp_dir() as tmp_dir_name: |
There was a problem hiding this comment.
What is this change all about?
There was a problem hiding this comment.
My usage of tempfile.NamedTemporaryFile() was failling on Windows GitHub actions (because of the path not being writable and having no fallback). So the creation of a temp memmap was moved to a function that works slightly differently (instead of creating a temp file using the library, it creates a temp file inside an already created temp_dir.
This is safer, in general, in case of usage on a cluster, shared machine or virtual machine (which is why it was failling on Windows tests), but this big change is just the same small changes for each kind of data store in TRX (which makes it look major)
Experiment with MagicMock, more to follow