Details
Details
ALCHEMY_API_KEY=alchemykeygoeshere yarn test utils/ens-cache.test.js
Two notes on the test plan:
- I originally wanted test cases for an ETH name that wasn't normalized and for a reverse resolution that didn't have a matching forward resolution, but I couldn't find anything in the wild. I found non-normalized ETH names but none of them had a reverse resolution set, and I didn't find any reverse resolutions without matching forward resolutions. I could construct these myself but it would probably require hitting some APIs directly or something, since I'm guessing ENS doesn't want you doing these things. Probably not worth it.
- The tests require ALCHEMY_API_KEY, otherwise they are silently ignored. This diff adds that environmental to GitHub Actions, but the tests will get ignored when run in precommit hooks and in Buildkite. Discussing with Atul about potentially making it work on Buildkite, but my impression is that it might not be worth the time.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Note: after landing, I should confirm GitHub Actions CI is actually running my test by checking the log
| lib/utils/ens-cache.js | ||
|---|---|---|
| 12–22 ↗ | (On Diff #20956) | Nit: I should move these types above the comment |
Comment Actions
Looks good. Both comments are super minor, feel free to ignore/land as-is.
| lib/utils/ens-cache.js | ||
|---|---|---|
| 12–13 ↗ | (On Diff #20960) | Wonder if lowercasedETHAddress would be clearer for future readers instead of having to jump here to figure out what "normalized" means in this context? But also normalized prefix is symmetrical with normalizedENSName below? Don't feel strongly, whatever you prefer |
| lib/utils/ens-cache.test.js | ||
| 20–49 ↗ | (On Diff #20960) | Might be missing something, but could we move the log inside the describe block and also early return before the it blocks? |