Skip to content

Conversation

jamesbraza
Copy link
Collaborator

#311 added some log parsing as a way of checking call ordering, and used a complicated fixture reset_log_levels to do so.

This code was confusing me a bit, so I just went ahead and cleaned it up to better use pytest's built-in caplog, with tighter assertions.

@jamesbraza jamesbraza requested a review from mskarlin July 10, 2025 23:25
@jamesbraza jamesbraza self-assigned this Jul 10, 2025
@Copilot Copilot AI review requested due to automatic review settings July 10, 2025 23:25
@jamesbraza jamesbraza added the enhancement New feature or request label Jul 10, 2025
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jul 10, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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 removes a custom logging fixture in favor of pytest’s built-in caplog, scopes log-level settings to the specific paperqa.clients logger, and tightens assertions around log record ordering.

  • Deleted the reset_log_levels fixture and unused logging import.
  • Updated tests to call caplog.set_level(logging.DEBUG, logger="paperqa.clients") instead of a global reset.
  • Changed record_indices to track multiple occurrences and added clearer assertions.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/test_clients.py Removed reset_log_levels usage, scoped caplog settings, and updated index-tracking logic for sequential log checks.
tests/conftest.py Removed the reset_log_levels fixture and the now-unused import logging.
Comments suppressed due to low confidence (1)

tests/test_clients.py:563

  • The assertion message is incorrect for semantic_scholar; it should read something like "Semantic Scholar should run" to match the key being checked.
        assert record_indices["semantic_scholar"], "Crossref should run"

@jamesbraza jamesbraza force-pushed the removing-log-fixture branch from 792fbac to bb9efb3 Compare July 10, 2025 23:26
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 11, 2025
@jamesbraza jamesbraza force-pushed the removing-log-fixture branch from bb9efb3 to fd9284a Compare July 11, 2025 17:13
@jamesbraza jamesbraza merged commit d9bf25e into main Jul 11, 2025
5 checks passed
@jamesbraza jamesbraza deleted the removing-log-fixture branch July 11, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants