Page MenuHomePhabricator

[alias-server] Verify database is appropriately populated in unit tests
ClosedPublic

Authored by bytesofman on Jul 3 2023, 23:15.

Details

Summary

T3060

These unit tests were created before the app had database functionality and were not modified when database functionality was added.

Add database verifications to existing unit tests. Use a custom server state for unit tests to avoid storing 100 megabytes of chronik tx history.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
patch-testing-bug
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24339
Build 48287: Build Diffalias-server-tests
Build 48286: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Jul 4 2023, 14:05
Fabien added a subscriber: Fabien.

I don't really understand the changes here.
You're testing the db status in tests for the chronik handler, the events handler (is that different ?) and the main app (same ?). you're duplicating the test intent over these tests and it's why it's causing all these duplicates.
Keep the things self contained and avoid yourself the exponential maintenance cost.

apps/alias-server/test/chronikWsHandlerTests.js
49 ↗(On Diff #41227)

Macro whatyearisit:

apps/alias-server/test/mainTests.js
52 ↗(On Diff #41227)

Why is this duplicated everywhere ?

This revision now requires changes to proceed.Jul 4 2023, 14:05

I don't really understand the changes here.

You're testing the db status in tests for the chronik handler, the events handler (is that different ?) and the main app (same ?). you're duplicating the test intent over these tests and it's why it's causing all these duplicates.
Keep the things self contained and avoid yourself the exponential maintenance cost.

Ultimately, these functions are supposed to update the database. I think the best way to avoid the expanding maintenance cost is to require the top level functions to "return a callback" ... then the unit test for example of main.js is just looking "did it call the expected function" and not "did the entire chain of updating the database work out as we've expected." This tho also gets complicated, bc then we are loading up all these parent functions with numerous callbacks.

Root cause of the diff is I need to show that handleBlockConnected clears out pendingAliases that have confirmed. In writing this unit test, I noticed that it failed because the unprocessConfirmedTxs were not correct, due to the server state issue. So, rather than just add one test where it's "fixed" to show this pending alias cleared behavior, fixing it everywhere.

I'm open to alt approaches but imo the complication of passing callbacks is a little worse than the current situation, where we are perhaps over-testing.

apps/alias-server/test/mainTests.js
52 ↗(On Diff #41227)

I could drop the comments everywhere. Would still need the serverstate update in before for relevant files.

Could also combine all the files and then only need it once, but imo there is some benefit to having unit tests in matching files.

remembering the current year

I'm open to alt approaches but imo the complication of passing callbacks is a little worse than the current situation, where we are perhaps over-testing.

This callback idea is horrible, at least we agree on that.
I think you're making the tests over complicated. You're not testing the database (and should not) but alias registration.
So you need 2 distinct things:

  • Unit tests for the functions that have some level of logic. This is usually done by crafting special inputs to make sure you get 100% coverage (or be as close to 100% as possible).
  • Integration tests which is mostly what you're doing here, and the easiest way to do so is to use the API => input blocks via chronik, and retrieve the aliases via the /alias endpoint. This roundtrip can be repeated for each edge case you want, including mocked inputs from the real world if that makes sense.

I have the feeling that this boundary is not well defined in your tests which will eventually lead to a maintenance nightmare.

Let's move with this change as it's not introducing the symptom, but you need to make sure your tests are aligned with this idea otherwise you will spend more time fixing and maintaining them than developing your application. It's maybe already the case.

This revision is now accepted and ready to land.Jul 4 2023, 16:32