MetaMask / MetaMask/core

Reorganize network client tests

Open
#1,542 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

team-wallet-framework wf-test-quality-improvement
Dominant language
TypeScript
Stars
413
Forks
308
Avg merge
1d 4h
Merged PRs (30d)
253

Description

We now have tests that exercise most of the behavior that MetaMask provides around various RPC methods via middleware — these are the "network client tests" — but some of this behavior lacks tests. Before we can backfill the remaining tests, however, we need to address some issues with the existing ones.

The tests are currently organized first by whether they are supported by Infura and/or whether they are contained in the Ethereum spec, and then by whether we assume that an RPC method takes a block parameter or not. The first level isn't necessary from a test perspective; it's just a factoid, but it doesn't affect behavior. The second level is important from a behavior perspective, but the behavior in question comes from the block cache middleware, and although that's the middleware that is most impactful, it is not the only way to categorize RPC methods given that there are other middleware that affect other RPC methods.

All this to say a better organizational scheme would be to use a flat structure. I propose the following guidelines:

* There should be one `describe` per RPC method, with the behavior for that RPC method tested within that `describe`.
* All tests should be named properly to reflect the behavior that they are testing. This means if we have a test where expectations differ slightly based on the RPC method being used, the type of network client being tested, or some other condition, that test should be split into multiple instances.
* It should be easy to find the tests for a particular network client / RPC method combo by starting at the top of a file and scrolling down. With so many tests, it is confusing to have to jump between multiple files to be able to read a whole test or even a whole suite of tests. This means that `if` statements are allowed to specify differences in behavior but only if it does not impact readability.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Locate the existing network client tests and first inventory their describe blocks and RPC-method groupings. Run the relevant test suite before reorganizing it. Done means one describe per RPC method, clearly named behavior tests, and each network client/RPC method combination readable by scrolling through a single file.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
api, testing
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.