Skip to content

fix(test): Check method when comparing matchers #430

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

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

maxi297
Copy link
Contributor

@maxi297 maxi297 commented Mar 18, 2025

What

This test (and others but I'm using this one as an example) were failing after updating the CDK version. The reason is that the polling and delete requests are identical except from the method.

How

So maybe HttpRequest should have the method but it does not seem like an easy change for now as this would be a breaking change (i.e. requiring the method in the HttpRequest) and it also goes against how the HttpMocker was structure by having one method per method. Those are probably errors from the past and things we would do differently in the future but eh...!

So for now, I've just:

  • added the method in the comparison in HttpMocker
  • matched on the reference and not the eq in assert_number_of_calls

Summary by CodeRabbit

  • Refactor

    • Enhanced internal HTTP request handling by organizing requests by method, leading to more precise matching and clearer error reporting.
  • Tests

    • Improved and expanded test scenarios to ensure that requests with varying HTTP methods are validated correctly, boosting overall system reliability.

@maxi297
Copy link
Contributor Author

maxi297 commented Mar 18, 2025

/autofix

Auto-Fix Job Info

This job attempts to auto-fix any linting or formating issues. If any fixes are made,
those changes will be automatically committed and pushed back to the PR.

Note: This job can only be run by maintainers. On PRs from forks, this command requires
that the PR author has enabled the Allow edits from maintainers option.

PR auto-fix job started... Check job output.

🟦 Job completed successfully (no changes).

@github-actions github-actions bot added the bug Something isn't working label Mar 18, 2025
Copy link
Contributor

coderabbitai bot commented Mar 18, 2025

📝 Walkthrough

Walkthrough

The pull request updates the HttpMocker class by restructuring the storage of HTTP request matchers. The matchers are now maintained in a dictionary keyed by HTTP methods instead of a simple list. A new helper method _get_matchers has been added to aggregate matchers across all methods, and associated methods have been updated to use this new structure. Additionally, test cases have been renamed for clarity and extended to cover scenarios where the same request is mocked with different HTTP methods.

Changes

File Path Change Summary
airbyte_cdk/test/mock_http/mocker.py Reworks matcher storage from a list to a dict mapping HTTP methods to matcher lists; adds _get_matchers method; updates _validate_all_matchers_called, _mock_request_method, and assert_number_of_calls to use the new structure; improves error handling.
unit_tests/test/mock_http/test_mocker.py Renames test method from "decorate" to "mock" terminology; adds a new test to ensure that using different HTTP methods for the same request does not raise an exception.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant HttpMocker
    participant MatcherStore as "HTTP Matcher Store (dict)"
    Client->>HttpMocker: Call mock_request_method(new_matcher with method\n(e.g., GET))
    HttpMocker->>MatcherStore: Retrieve matchers for GET
    alt Matcher exists?
        MatcherStore-->>HttpMocker: Existing matcher(s)
        HttpMocker->>Client: Raise ValueError("Matcher already exists")
    else
        MatcherStore-->>HttpMocker: No conflict
        HttpMocker->>MatcherStore: Append new_matcher to GET list
        HttpMocker->>Client: Return success response
    end
Loading
sequenceDiagram
    participant Validator
    participant HttpMocker
    participant MatcherStore as "HTTP Matcher Store (dict)"
    Validator->>HttpMocker: Invoke _validate_all_matchers_called
    HttpMocker->>MatcherStore: Retrieve all matchers via _get_matchers
    MatcherStore-->>HttpMocker: Return matcher list
    HttpMocker->>Validator: Confirm all matchers have been called
Loading

Possibly related PRs

Suggested labels

bug, airbyte-python-cdk, airbyte-python-cdk/low-code/http-requester

Suggested reviewers

  • darynaishchenko
  • bazarnov
  • aaronsteers

Would you like to add these reviewers to the PR, wdyt?

Tip

⚡🧪 Multi-step agentic review comment chat (experimental)
  • We're introducing multi-step agentic chat in review comments. This experimental feature enhances review discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments.
    - To enable this feature, set early_access to true under in the settings.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 923acc2 and acdae8d.

📒 Files selected for processing (2)
  • airbyte_cdk/test/mock_http/mocker.py (7 hunks)
  • unit_tests/test/mock_http/test_mocker.py (1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
unit_tests/test/mock_http/test_mocker.py (1)
airbyte_cdk/test/mock_http/mocker.py (3) (3)
  • HttpMocker (25:185)
  • get (94:95)
  • delete (110:113)
⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Analyze (python)
🔇 Additional comments (11)
airbyte_cdk/test/mock_http/mocker.py (9)

5-5: Nice addition of defaultdict import

Using defaultdict is a clean approach for managing collections of matchers by HTTP method - it eliminates the need for explicit dictionary initialization checks.


8-8: Proper type hint updates

Good addition of Dict and Iterable to the imports to support the new data structure and matcher retrieval method. The type hints provide clear documentation of the interface changes.


44-44: Well-designed data structure change

Changing from a flat list to a dictionary keyed by HTTP methods is an elegant solution to allow the same request to be mocked with different HTTP methods. Using defaultdict(list) ensures you don't need to handle the case when a method key doesn't exist yet.


59-61: Good adaptation of the validation logic

The validation now properly iterates through all matchers regardless of HTTP method by using the new _get_matchers helper. This maintains the existing functionality while accommodating the new data structure.


73-75: Key fix to allow different HTTP methods

This change is the heart of the fix - checking for duplicates only within the same HTTP method, not across all methods. This allows the same request to be mocked with different HTTP methods (like GET and DELETE) which solves the polling/delete issue mentioned in the PR objectives.


133-134: Good use of the new helper method

Using the _get_matchers helper ensures consistent behavior when counting calls across all HTTP methods. The use of matcher.request is request compares by reference instead of equality which aligns with the PR objectives.


154-155: Consistent use of the helper method

Good update to use _get_matchers in the error handling path as well. This ensures that error messages include all matchers regardless of HTTP method.


179-182: Well-implemented helper method

The _get_matchers method is a clean solution for flattening the dictionary of matchers into a single iterable. Using yield from is elegant and memory-efficient. This helper keeps the code DRY by centralizing the iteration logic.


185-185: Consistent cleanup method

Good update to the clear_all_matchers method to use defaultdict(list) for consistency with the initialization. This ensures the data structure remains in the expected state after clearing.

unit_tests/test/mock_http/test_mocker.py (2)

298-305: Clear test rename

Renaming from "decorate" to "mock" makes the test name more accurately reflect what's being tested. This improves readability and maintainability of the test suite.


306-311: Excellent test coverage for the new functionality

This new test case directly validates the core feature added in this PR - ensuring that the same request can be mocked with different HTTP methods. It's a concise and focused test that verifies the solution works as expected for the problem described in the PR objectives.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@maxi297 maxi297 merged commit c4d0f91 into main Mar 18, 2025
26 checks passed
@maxi297 maxi297 deleted the maxi297/fix-http-mocker branch March 18, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants