-
Notifications
You must be signed in to change notification settings - Fork 79
Test review #1696
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
Test review #1696
Conversation
* Add integraiton test * Passing * Passing * Update tests * Update CHANGELOG * Update CHANGELOG.md
* Passing * Added IframeProviderStrategy tests * Update CHANGELOG * Update CHANGELOG.md
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR adds comprehensive unit test suites for ExtensionProviderStrategy and IframeProviderStrategy covering initialization, authentication, and transaction/message signing flows. It updates the mock server with a new transactions endpoint, adds test environment polyfills for setImmediate/clearImmediate compatibility, and updates the changelog and coverage metrics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @arhtudormorar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the test coverage and reliability of the provider strategies within the application. It introduces dedicated unit tests for the ExtensionProviderStrategy and IframeProviderStrategy, ensuring robust functionality for user authentication and transaction handling. Additionally, it includes minor updates to the changelog and test setup to reflect these improvements. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds tests for IframeProviderStrategy and ExtensionProviderStrategy, which significantly increases test coverage. The changes are well-implemented. I've added a few suggestions to improve code quality and maintainability, focusing on a potentially confusing type name, improving type safety in tests by avoiding as any, and a minor formatting fix in the changelog.
src/providers/strategies/IframeProviderStrategy/tests/IframeProviderStrategy.test.ts
Show resolved
Hide resolved
src/providers/strategies/IframeProviderStrategy/tests/IframeProviderStrategy.test.ts
Show resolved
Hide resolved
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/providers/strategies/ExtensionProviderStrategy/tests/ExtensionProviderStrategy.test.ts (1)
71-75: Avoid leaking theinitSignStatespy into future tests
jest.spyOn(ExtensionProviderStrategy.prototype as any, 'initSignState')permanently replaces the implementation for the rest of this file. That’s fine today, but could surprise future tests that rely on the realinitSignState. Consider restoring it after the assertion:const initSignStateSpy = jest .spyOn(ExtensionProviderStrategy.prototype as any, 'initSignState') .mockResolvedValue({ manager: { closeUI: jest.fn() } }); try { // test body } finally { initSignStateSpy.mockRestore(); }This keeps the test self‑contained.
src/providers/strategies/IframeProviderStrategy/tests/IframeProviderStrategy.test.ts (1)
167-177: Align test name with assertions for the sign‑error pathThe
"cancels action and rethrows on sign error"test currently only asserts thatsignTransactionsrejects with'sign error'; it doesn’t check thatcancelAction,onClose, orcloseUIwere invoked.Either:
- Add expectations for these side‑effects (e.g.,
expect(mockIframeProvider.cancelAction).toHaveBeenCalled()/expect(handlers.manager.closeUI).toHaveBeenCalled()), or- Rename the test to focus solely on the rethrow behavior.
That will keep the spec and behavior in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)coverage-total.json(1 hunks)src/__mocks__/server.ts(1 hunks)src/providers/strategies/ExtensionProviderStrategy/tests/ExtensionProviderStrategy.test.ts(1 hunks)src/providers/strategies/IframeProviderStrategy/tests/IframeProviderStrategy.test.ts(1 hunks)src/setupTests.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/__mocks__/server.ts (1)
src/__mocks__/accountConfig.ts (1)
testNetwork(7-8)
src/providers/strategies/IframeProviderStrategy/tests/IframeProviderStrategy.test.ts (4)
src/__mocks__/data/storeData/network.ts (1)
network(1-20)src/__mocks__/data/storeData/account.ts (1)
account(4-27)src/providers/types/providerFactory.types.ts (1)
ProviderTypeEnum(26-35)src/__mocks__/accountConfig.ts (1)
testAddress(4-5)
src/providers/strategies/ExtensionProviderStrategy/tests/ExtensionProviderStrategy.test.ts (5)
src/__mocks__/accountConfig.ts (1)
testAddress(4-5)src/providers/ProviderFactory.ts (1)
ProviderFactory(31-149)src/providers/strategies/ExtensionProviderStrategy/ExtensionProviderStrategy.ts (1)
ExtensionProviderStrategy(13-95)src/lib/sdkCore.ts (3)
Transaction(7-7)Message(12-12)Address(10-10)src/__mocks__/data/mockPendingTransaction.ts (1)
mockPendingTransaction(3-23)
⏰ 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). (1)
- GitHub Check: run_template_dapps_integration_script
🔇 Additional comments (4)
coverage-total.json (1)
4-4: Coverage badge update looks goodThe new
87.98%value is a straightforward metadata update; no issues from a code perspective.CHANGELOG.md (1)
8-10: Changelog entries match the test additionsThe new “Unreleased” items correctly reference the Iframe and Extension provider test PRs and keep the history consistent.
src/__mocks__/server.ts (1)
57-60: PPU mock endpoint is consistent and scopedThe new
/transactions/ppu/0MSW handler has the expected shape (lastBlock,fast,faster) and usestestNetwork.apiAddress, so it should integrate cleanly with tests that rely on gas-station metadata.src/setupTests.js (1)
23-26: setImmediate/clearImmediate polyfill is safe for the test envConditionally mapping
setImmediate/clearImmediatetosetTimeout/clearTimeoutwhen missing is a pragmatic fix for Jest/JS DOM environments and won’t affect Node wheresetImmediatealready exists.
src/providers/strategies/IframeProviderStrategy/tests/IframeProviderStrategy.test.ts
Show resolved
Hide resolved
* Add getTransactionsHistory tests * Added handleSignError tests * Partially working * update * Passing * Passing * Passing * Passing * Passing * Passing * Update CHANGELOG * Update CHANGELOG * Update coverage
Signed-off-by: bigmoonbit <bigmoonbit@outlook.com> Co-authored-by: Tudor Morar <tudor.morar@multiversx.com>
* Extend tests * Add DappProvider tests * Extend tests * Add CONTRIBUTING.md
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.