[Cashtab] Convert agora tests to ts
ClosedPublic
Actions

Authored by bytesofman on Jul 3 2025, 23:07.

Details

Reviewers

emack

Group Reviewers

Restricted Project

Commits

rABC3dc4ba40ba96: [Cashtab] Convert agora tests to ts

Summary

Want to standardize all of Cashtab tests. Use ts and standardized test helpers.

Note: this is almost entirely AI-generated (cursor)

imo this is one of the areas it is best at, large scale refactor that is mostly type checking across lots of mocks, the kind of tedious improvement that risks otherwise being postponed indefinitely.

That said, still saw some weird behavior, like some outputScripts in mocks changed. Still does need to be reviewed. But changing only the test files and getting the same behavior but with typescript is imo a win.

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Branch

cashtab-more-ts

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 33755
Build 66988: Build Diff	cashtab-tests
Build 66987: arc lint + arc unit

Event Timeline

bytesofman created this revision.Jul 3 2025, 23:07

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 3 2025, 23:07

Harbormaster completed remote builds in B33753: Diff 54681.Jul 3 2025, 23:07

lint

Harbormaster completed remote builds in B33755: Diff 54683.Jul 3 2025, 23:29

back out AI-generated mystery changes to the mocks

Harbormaster completed remote builds in B33756: Diff 54684.Jul 4 2025, 00:04

bytesofman published this revision for review.Jul 4 2025, 00:06

bytesofman edited the summary of this revision. (Show Details)

emack accepted this revision.Jul 4 2025, 05:59

emack added a subscriber: emack.

emack added inline comments.

cashtab/src/components/Agora/__tests__/index.test.tsx
127 ↗	(On Diff #54684)	All this time I've been clearing it the dumb way as well, did Cursor pick up on this?
230 ↗	(On Diff #54684)	Normally you'd be getting me to back out unnecessary carriage returns, I assume this is just the new lint config?
309 ↗	(On Diff #54684)	AI can add contextual comments??
986 ↗	(On Diff #54684)	Is there an actual performance benefit to checking `toBeGreaterThan(0)` rather than just seeing if it's in the document?

This revision is now accepted and ready to land.Jul 4 2025, 05:59

bytesofman marked 4 inline comments as done.Jul 4 2025, 12:37

bytesofman added inline comments.

cashtab/src/components/Agora/__tests__/index.test.tsx
127 ↗	(On Diff #54684)	in this case I think cursor was just defaulting to the normal way vs Cashtab's custom way, which comes from a custom function The cashtab function wasn't added for no reason -- was part of an incremental troubleshooting effort when we first added all of these tests. Possibly we still need to use the cashtab custom function in some test files that clear and add to storage multiple times. Mb tho we can just use the .clear() everywhere now. The main determinant is if the tests work and confirm expected behavior or not, vs if it is for whatever reason not fully or always clearing storage between tests. But the leaky behavior between tests could be something that was happening for other reasons more than a year ago.
230 ↗	(On Diff #54684)	we do not have a lint config set up for this. since the whole file was created with this convention of "we include line breaks between tests" -- imo it's fine
309 ↗	(On Diff #54684)	Yes although they are often totally incorrect about the reason provided
986 ↗	(On Diff #54684)	Yes. AI came up with this one, but it is a better test. We are just trying to test that we see the value on the screen. In this case, we actually expect to see the value in multiple places. So we can't use react testing library getter to check for just one (will get an error / test failure). But we also don't really care / are not testing in this instance about multiple appearances. So this would make it less flaky. We are effectively just seeing if it's in the document. If we don't use this, then we have to use .toHaveLength(3) or something, which can be flakier as the other divs may not appear at the same time.