Merged
Conversation
Co-authored-by: derek <derek@fireworks.ai>
|
Cursor Agent can help with this pull request. Just |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: Pull Request
about: Propose changes to the codebase
title: "Fix:
test_ensure_loggingfailure in CI due to disabled SQLite logger"labels: ''
assignees: ''
Description
This PR fixes a flaky CI test,
tests/pytest/test_pytest_ensure_logging.py::test_ensure_logging, which was failing because the SQLite logger was inadvertently disabled by theDISABLE_EP_SQLITE_LOGenvironment variable in the CI environment.The test mocks
SqliteEvaluationRowStoreand asserts that itsupsert_rowmethod is called. However, whenDISABLE_EP_SQLITE_LOGwas set to "1" (as it was in CI), theNoOplogger was initialized instead ofSqliteDatasetLoggerAdapter, preventingupsert_rowfrom ever being called on the mock.The fix involves explicitly setting
DISABLE_EP_SQLITE_LOG="0"within the test usingmonkeypatch.setenvand resetting the lazy logger cache (default_logger._logger = None). This ensures theSqliteDatasetLoggerAdapteris correctly initialized and used during the test, allowingupsert_rowto be called as expected. This change only affects the test environment and does not alter any production logic.Fixes # (issue)
Implements # (issue)
Type of change
How Has This Been Tested?
The fix was verified by running the specific failing test locally after applying the changes.
pytest -q tests/pytest/test_pytest_ensure_logging.py::test_ensure_loggingTest Configuration:
Checklist:
black .,isort .,flake8 .)Screenshots (if applicable)
N/A
Additional context
This change is purely for test robustness in CI and does not impact the application's runtime behavior or logging configuration outside of this specific test.