Skip to content
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: injected providers #369

Closed
wants to merge 8 commits into from
Closed

test: injected providers #369

wants to merge 8 commits into from

Conversation

0xisk
Copy link
Member

@0xisk 0xisk commented May 16, 2023

Explanation

Unit-testing for:

  • Providers.
  • EventEmitter service

More Information

Screenshots/Screencaps

N/A

Manual Testing Steps

N/A

Pre-Merge Checklist

  • PR template is filled out
  • IF this PR fixes a bug, a test that would have caught the bug has been added
  • PR is linked to the appropriate GitHub issue
  • PR has been added to the appropriate release Milestone

PR template source from github.com/MetaMask

@0xisk 0xisk marked this pull request as ready for review May 17, 2023 12:10
@0xisk 0xisk requested a review from AtHeartEngineer as a code owner May 17, 2023 12:10
@0xisk 0xisk removed the request for review from AtHeartEngineer May 17, 2023 12:11
@0xisk 0xisk self-assigned this May 17, 2023
@0xisk 0xisk added the 🕵️ testing Troubleshooting testing issues label May 17, 2023
@0xisk 0xisk added this to the Successful Beta v0.0.1 Release milestone May 17, 2023
@0xisk 0xisk requested a review from 0xmad May 17, 2023 14:19
Copy link
Member

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xisk thanks, there are some comments regarding tests.

@@ -3,7 +3,7 @@ import { Emitter, createNanoEvents } from "nanoevents";
import { Events, EventHandler, EventName } from "./types";

export default class EventEmitter {
private emitter: Emitter<Events>;
emitter: Emitter<Events>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep it private


beforeEach(() => {
// Mocking private dependencies
const mockTryInject = jest.spyOn(CryptKeeperInjectedProvider.prototype as any, "tryInject");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private dependencies is not the same as private methods.
Here you need to spy on window.postMessage and check that it has been called with specific params

/**
* @jest-environment jsdom
*/
/* eslint @typescript-eslint/no-explicit-any: 0 */ // --> OFF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use any for new code.

Copy link
Member

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xisk thanks, there are some comments regarding tests.

@0xmad 0xmad changed the title Test/injected providers test: injected providers May 18, 2023
@0xisk 0xisk force-pushed the test/injected-providers branch from 5b12089 to c6b8a22 Compare June 2, 2023 15:09
@0xisk 0xisk modified the milestones: v0.1.0-beta, v0.2.0-beta Aug 20, 2023
@0xmad
Copy link
Member

0xmad commented Sep 1, 2023

These changes are related to old project structure so they are not applicable for monorepo.

@0xmad 0xmad closed this Sep 1, 2023
@0xmad 0xmad deleted the test/injected-providers branch September 6, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕵️ testing Troubleshooting testing issues
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

test: writing testing for #335
2 participants