Skip to content

test: add more tests for FitPlugin.fit#888

Merged
tbouffard merged 4 commits intomainfrom
test/more_fit_tests
Jul 28, 2025
Merged

test: add more tests for FitPlugin.fit#888
tbouffard merged 4 commits intomainfrom
test/more_fit_tests

Conversation

@tbouffard
Copy link
Member

@tbouffard tbouffard commented Jul 25, 2025 •
edited
Loading

Test options: border, ignore height, ignore width, keepOrigin, margin

Summary by CodeRabbit

  • Tests
    • Improved clarity in test variable naming for better readability.
    • Added new test cases covering scale limits, ignoring dimensions, special cases, and options like border, keep origin, and margin when fitting content.
  • Refactor
    • Simplified fit method with early returns and clearer control flow without changing core functionality.

@tbouffard
test: add tests for FitPlugin.fit
156799c 
Test options: border, keepOrigin, margin
@tbouffard tbouffard added the chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) label Jul 25, 2025
@coderabbitai
Copy link

coderabbitai bot commented Jul 25, 2025 •
edited
Loading

Walkthrough

The FitPlugin was refactored to improve the fit method by introducing early returns for invalid containers or graph bounds and reducing nested conditionals. The test suite was renamed for clarity and expanded with numerous new test cases covering edge cases and additional options for the fit and fitCenter methods. No changes were made to exported or public entity declarations.

Changes

File(s) Change Summary
FitPlugin source
packages/core/src/view/plugins/FitPlugin.ts		 Refactored fit method to use early returns and reduce nesting; clarified control flow and return paths without changing core logic.
FitPlugin tests
packages/core/__tests__/view/plugins/FitPlugin.test.ts		 Renamed Jest spy variable for clarity; expanded test coverage with nested describes and new cases for scale limits, margins, keepOrigin, ignoring dimensions, and special edge cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.

📜 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 156799c and 9ea3615.

📒 Files selected for processing (2)
  • packages/core/__tests__/view/plugins/FitPlugin.test.ts (4 hunks)
  • packages/core/src/view/plugins/FitPlugin.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings 
Learnt from: tbouffard
PR: maxGraph/maxGraph#715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the `fit` method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.

Learnt from: tbouffard
PR: maxGraph/maxGraph#857
File: packages/core/src/view/plugins/SelectionHandler.ts:1081-1092
Timestamp: 2025-06-26T13:27:19.012Z
Learning: User tbouffard prefers to keep PRs focused on their main objective and defer implementation improvements or logic fixes to future dedicated PRs. For maxGraph, refactoring PRs like Dictionary→Map migration should not include unrelated logic improvements even when valid issues are identified in the migrated code.

Learnt from: tbouffard
PR: maxGraph/maxGraph#776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.

Learnt from: tbouffard
PR: maxGraph/maxGraph#849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
packages/core/src/view/plugins/FitPlugin.ts (1)

Learnt from: tbouffard
PR: #715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the fit method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.

packages/core/__tests__/view/plugins/FitPlugin.test.ts (5)

Learnt from: tbouffard
PR: #582
File: packages/core/tests/view/handler/config.test.ts:0-0
Timestamp: 2024-11-24T17:28:08.023Z
Learning: In packages/core/__tests__/view/handler/config.test.ts, when writing Jest tests in TypeScript for configuration reset functions, small duplication is acceptable if it maintains test clarity and readability. Avoid refactoring tests in ways that could make them harder to understand, especially when future use cases might require specific checks.

Learnt from: tbouffard
PR: #715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the fit method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.

Learnt from: tbouffard
PR: #826
File: packages/js-example-nodejs/src/index.cjs:64-69
Timestamp: 2025-05-13T12:54:55.231Z
Learning: For example code in the maxGraph repository, maintainers prefer to keep scripts simple without error handling to focus on demonstrating core functionality, especially in demonstration scripts like those in packages/js-example-nodejs.

Learnt from: tbouffard
PR: #791
File: packages/ts-example/vite.config.js:30-30
Timestamp: 2025-04-29T13:25:31.494Z
Learning: In the maxGraph project, each example package (ts-example, ts-example-selected-features, ts-example-without-defaults) implements different use cases with varying features, resulting in different application sizes. Therefore, each package has its own specific chunkSizeWarningLimit value in its vite.config.js file, calibrated to its expected bundle size.

Learnt from: tbouffard
PR: #598
File: packages/website/docs/manual/getting-started.md:70-70
Timestamp: 2024-12-15T18:19:56.236Z
Learning: In code examples within the documentation, such as in packages/website/docs/manual/getting-started.md, we assume that the graph-container element exists and is an HTMLElement, and we avoid adding error handling for its initialization to keep the code simple.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: build
  • GitHub Check: build (macos-14)
  • GitHub Check: build (windows-2022)
  • GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (8)
packages/core/src/view/plugins/FitPlugin.ts (4)

140-146: LGTM: Good refactoring with early returns.

The early returns for missing container and invalid graph bounds improve code readability by reducing nesting. The conditions correctly handle edge cases where fitting is not possible.

148-160: LGTM: Clear variable naming and logic organization.

The variable names (w1, h1 for container dimensions and later w2, h2 for graph dimensions) are clear in context. The keepOrigin handling is properly implemented by adjusting the bounds to start from origin.

162-187: LGTM: Scale calculation logic is sound.

The scale calculation properly handles:

  • Original scale normalization
  • Background image size consideration
  • Border and margin adjustments
  • Min/max scale clamping
  • Conditional logic for ignoring width/height

188-230: LGTM: Improved control flow for scaling operations.

The refactored control flow clearly separates the different scaling scenarios:

  • When keepOrigin is false: proper translation calculation with scrollbar handling
  • When keepOrigin is true: simple scale setting
  • When enabled is false: returning calculated scale without applying it

The logic correctly preserves the original behavior while improving readability.

packages/core/__tests__/view/plugins/FitPlugin.test.ts (4)

54-62: LGTM: Good test setup with initial scale verification.

The test correctly sets an initial scale and verifies it's returned unchanged when graph has zero dimensions. The spy verification ensures scaleAndTranslate is called with the original scale.

72-78: LGTM: Spy rename addresses previous feedback.

The spy has been renamed from viewMock to scaleAndTranslateSpy as requested in previous reviews, making the purpose clearer.

117-248: LGTM: Excellent test organization with comprehensive coverage.

The test restructuring with nested describe blocks addresses previous feedback perfectly:

  • "no ignored dimensions" groups the main test cases
  • Individual tests for "ignore width" and "ignore height"
  • "special cases" for edge conditions

The new test cases provide excellent coverage for:

  • Scale limiting (minFitScale/maxFitScale)
  • Border and margin options
  • keepOrigin behavior (correctly verifies setScale vs scaleAndTranslate)
  • All scenarios have proper spy verification

282-322: LGTM: Comprehensive edge case testing.

The special cases section excellently tests the early return scenarios from the refactored fit method:

  • No container case properly hacks around the constructor limitation
  • Parameterized test for non-positive width/height dimensions
  • Both cases correctly verify no scaling methods are called and original scale is returned

This provides complete coverage for the new early return logic.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch test/more_fit_tests

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:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • 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 explain this code block.
    • @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 explain its main purpose.
    • @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.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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 generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests 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.

tbouffard
tbouffard commented Jul 27, 2025
View reviewed changes
tbouffard
tbouffard commented Jul 27, 2025
View reviewed changes
@tbouffard tbouffard changed the title test: add tests for FitPlugin.fit test: add moe tests for FitPlugin.fit Jul 27, 2025
@tbouffard tbouffard changed the title test: add moe tests for FitPlugin.fit test: add more tests for FitPlugin.fit Jul 27, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 28, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@tbouffard tbouffard merged commit 95cdbc8 into main Jul 28, 2025
7 checks passed
@tbouffard tbouffard deleted the test/more_fit_tests branch July 28, 2025 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@tbouffard