Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an initial commit with several new tests and functions to manage miniSEED record content, including sorting and comparison routines.
- Added tests for buffer comparison and MS3Record functionality.
- Introduced helper functions sort_mseed_content and compare_miniseed_content in the library.
- Consolidated sine wave sample generation into conftest for reuse across tests.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_msrecord_buffer_compare.py | Added fixtures and tests for comparing and sorting miniSEED content |
| tests/test_msrecord.py | Updated tests by removing unused imports and reusing sine_500 fixture |
| tests/conftest.py | Centralized sine wave sample generation for tests |
| src/mseedlib/msrecord_buffer_compare.py | Added sorting and comparison functions for miniSEED content |
| src/mseedlib/msrecord.py | Added record comparison functions and updated packing logic |
Comments suppressed due to low confidence (1)
src/mseedlib/msrecord_buffer_compare.py:38
- The docstring for compare_miniseed_content states it returns a boolean value, but the implementation returns a tuple (binary_compare_list, logical_compare_list). Consider updating the documentation or return type to accurately reflect the output.
def compare_miniseed_content(content1: bytes, content2: bytes, ignore_order: bool = True) -> [bool, dict]:
| def msr_XX_TEST__B_S_Y_bytes() -> tuple(MS3Record, bytes): | ||
| msr1 = MS3Record() | ||
| msr1.set_starttime_str("2023-01-02T01:02:03.123456789Z") | ||
| msr1.sourceid = "FDSN:XX_TEST__B_S_X" |
There was a problem hiding this comment.
In fixture msr_XX_TEST__B_S_Y_bytes, the sourceid is set to "FDSN:XX_TEST__B_S_X" instead of a value reflecting the intended unique identity (e.g. "FDSN:XX_TEST__B_S_Y").
| msr1.sourceid = "FDSN:XX_TEST__B_S_X" | |
| msr1.sourceid = "FDSN:XX_TEST__B_S_Y" |
| msr1.pack(record_handler, datasamples=sine_500, sampletype="i") | ||
| msr1_bytes = bytes(record_buffer) # Make a copy of the packed record | ||
| return msr1, msr1_bytes | ||
|
|
||
|
|
||
| def record_handler(record, handler_data): | ||
| """A callback function for MS3Record.set_record_handler() | ||
| Stores the record in a global buffer for testing | ||
| """ | ||
| print("Record handler called, record length: %d" % len(record)) | ||
| global record_buffer | ||
| record_buffer = bytes(record) |
There was a problem hiding this comment.
The record_handler modifies a global variable (record_buffer) for test state, which may lead to unexpected interference in concurrent test runs. Consider using a fixture-scoped or context-local variable to store the record bytes.
| msr1.pack(record_handler, datasamples=sine_500, sampletype="i") | |
| msr1_bytes = bytes(record_buffer) # Make a copy of the packed record | |
| return msr1, msr1_bytes | |
| def record_handler(record, handler_data): | |
| """A callback function for MS3Record.set_record_handler() | |
| Stores the record in a global buffer for testing | |
| """ | |
| print("Record handler called, record length: %d" % len(record)) | |
| global record_buffer | |
| record_buffer = bytes(record) | |
| handler_data = {} | |
| msr1.pack(record_handler, handler_data=handler_data, datasamples=sine_500, sampletype="i") | |
| msr1_bytes = handler_data["record_buffer"] # Make a copy of the packed record | |
| return msr1, msr1_bytes | |
| def record_handler(record, handler_data): | |
| """A callback function for MS3Record.set_record_handler() | |
| Stores the record in a context-local variable for testing | |
| """ | |
| print("Record handler called, record length: %d" % len(record)) | |
| handler_data["record_buffer"] = bytes(record) |
In progress...