-
Notifications
You must be signed in to change notification settings - Fork 20
Tests: ingester's log excerpt functions #1593
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
Tests: ingester's log excerpt functions #1593
Conversation
7390ffe to
d6eb455
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive unit test coverage for the log excerpt utility functions along with supporting test fixture data. The changes introduce tests for log excerpt upload, caching, processing, and extraction functionality.
- Adds test fixture data file containing mock constants for log excerpt testing
- Implements comprehensive test coverage for all functions in
log_excerpt_utils.pyincluding upload, caching, processing, and maintenance operations - Organizes tests into logical test classes covering successful paths, error cases, and edge conditions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backend/kernelCI_app/tests/unitTests/helpers/fixtures/log_excerpt_data.py | Introduces mock constants and test data for log excerpt testing |
| backend/kernelCI_app/tests/unitTests/commands/monitorSubmissions/log_excerpt_utils_test.py | Adds comprehensive unit tests for all log excerpt utility functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
backend/kernelCI_app/tests/unitTests/commands/monitorSubmissions/log_excerpt_utils_test.py
Outdated
Show resolved
Hide resolved
backend/kernelCI_app/tests/unitTests/commands/monitorSubmissions/log_excerpt_utils_test.py
Outdated
Show resolved
Hide resolved
| @patch("kernelCI_app.management.commands.helpers.log_excerpt_utils.VERBOSE", False) | ||
| @patch("kernelCI_app.management.commands.helpers.log_excerpt_utils.CACHE_LOGS") | ||
| @patch("kernelCI_app.management.commands.helpers.log_excerpt_utils.cache_logs_lock") | ||
| def test_set_in_cache(self, mock_lock, mock_cache): | ||
| """Test setting a value in cache.""" | ||
| mock_lock.__enter__ = MagicMock() | ||
| mock_lock.__exit__ = MagicMock() | ||
|
|
||
| set_in_cache(EXCERPT_HASH_MOCK, LOG_EXCERPT_MOCK) | ||
|
|
||
| mock_cache.__setitem__.assert_called_once_with( | ||
| EXCERPT_HASH_MOCK, LOG_EXCERPT_MOCK | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: as we are always using one class per function, i would move this to another class
| @patch("kernelCI_app.management.commands.helpers.log_excerpt_utils.CACHE_LOGS") | ||
| @patch("kernelCI_app.management.commands.helpers.log_excerpt_utils.cache_logs_lock") | ||
| def test_get_from_cache_existing_key(self, mock_lock, mock_cache): | ||
| """Test getting an existing key from cache.""" | ||
| mock_cache.get.return_value = LOG_EXCERPT_MOCK | ||
| mock_lock.__enter__ = MagicMock() | ||
| mock_lock.__exit__ = MagicMock() | ||
|
|
||
| result = get_from_cache(EXCERPT_HASH_MOCK) | ||
|
|
||
| mock_cache.get.assert_called_once_with(EXCERPT_HASH_MOCK) | ||
| assert result == LOG_EXCERPT_MOCK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for this test and the ones below refered to cache utils, i think it would be better to add the key on the cache and then try to get after to see if the function returns the value correctly
Something like:
mock_cache.set(key, value)
result = mock_cache.get(key)
assert result == valuewith the current implementation, if we have:
def get_from_cache(log_hash: str) -> Optional[str]:
"""
Check if log_hash is in the cache
Returns:
str|None: The log_excerpt if it exists in cache, None otherwise
"""
with cache_logs_lock:
return CACHE_LOGS.get("somehash")the tests would still pass, but the logic for the cache function itself is incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I believe I should edit the test for a success get from cache. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated both get and set cache tests
| mock_set_log_excerpt_ofile, | ||
| mock_set_in_cache, | ||
| mock_get_from_cache, | ||
| mock_cache_logs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mock_cache_logs seems to be unused, assert or remove it maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't needed, removed
d6eb455 to
29c746c
Compare
gustavobtflores
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes
upload_logexcerpt
get_from_cache
set_in_cache
set_log_excerpt_ofile
process_log_excerpt_from_item
extract_log_excerpt
cache_logs_maintenance
How to test
Run the unit tests of the file and check that they are all passing.
Also check if the test cases cover the function well
Part of #1569