Get the new CVR export functions in libs/backend up to 100% code coverage #4090
Conversation
| import { configureApp } from '../../../test/helpers/shared_helpers'; | ||
| import { scanBallot, withApp } from '../../../test/helpers/custom_helpers'; | ||
| import { configureApp } from '../test/helpers/shared_helpers'; | ||
| import { scanBallot, withApp } from '../test/helpers/custom_helpers'; |
There was a problem hiding this comment.
An unrelated change. Just had a hunch that app_config.test.ts wasn't intentionally located inside apps/scan/backed/src/scanners/custom/. The other tests in apps/scan/backed/src/scanners/custom/ are all about scanning and prefixed with app_scan_*. Moved it up alongside other more general tests in apps/scan/backend/src/
There was a problem hiding this comment.
It was not intentionally there, it's bothered me but I hadn't moved it. Thanks!
| @@ -74,6 +75,7 @@ export function createMockUsbDrive(): MockUsbDrive { | |||
| removeUsbDrive() { | |||
| mockUsbTmpDir?.removeCallback(); | |||
| mockUsbTmpDir = undefined; | |||
| usbDrive.status.reset(); | |||
There was a problem hiding this comment.
Without these resets, inserting and removing the mock USB drive continues growing the list of expected repeated calls. This results in unexpected behavior like the mock USB drive indicating that it's no drive even after calling mockUsbDrive.insertUsbDrive
There was a problem hiding this comment.
Again, thank you. @jonahkagan and I been discussing this behavior and had decided to live with it, but I like this approach.
It's really only a problem on removing, because on insertion, there should always be some status polling. Otherwise, why insert the USB drive? But I'm fine resetting on both for consistency.
| const exportDirectoryName = scannerStore.getExportDirectoryName(); | ||
| if (!exportDirectoryName) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
In case you're wondering why this check has moved location and gone from checking false to true, this check, at its previous location, was almost equivalent to the ballotsCounted === 0 check (since we create the export directory after the first ballot is counted).
Now, by moving it after the ballotsCounted === 0 case, we're catching the case that we fail to create the export directory (and specifically save its path in the DB) after the first ballot is counted. Should be exceedingly rare but, regardless, the new check is more useful than the previous one.
If you want to get extra specific, technically the areOrWereCastVoteRecordsBeingExportedToUsbDrive(usbDriveStatus) would likely also catch this. But that check involves a marker file on the USB drive. If the USB drive were replaced with a blank USB drive, that check wouldn't catch anything (other checks after would).
Zooming out, we're talking about the edgiest of edge cases here
arsalansufi
requested review from
eventualbuddha
and removed request for
a team,
jonahkagan and
eventualbuddha
| import { configureApp } from '../../../test/helpers/shared_helpers'; | ||
| import { scanBallot, withApp } from '../../../test/helpers/custom_helpers'; | ||
| import { configureApp } from '../test/helpers/shared_helpers'; | ||
| import { scanBallot, withApp } from '../test/helpers/custom_helpers'; |
There was a problem hiding this comment.
It was not intentionally there, it's bothered me but I hadn't moved it. Thanks!
| @@ -74,6 +75,7 @@ export function createMockUsbDrive(): MockUsbDrive { | |||
| removeUsbDrive() { | |||
| mockUsbTmpDir?.removeCallback(); | |||
| mockUsbTmpDir = undefined; | |||
| usbDrive.status.reset(); | |||
There was a problem hiding this comment.
Again, thank you. @jonahkagan and I been discussing this behavior and had decided to live with it, but I like this approach.
It's really only a problem on removing, because on insertion, there should always be some status polling. Otherwise, why insert the USB drive? But I'm fine resetting on both for consistency.
| const exportDirectoryName = scannerStore.getExportDirectoryName(); | ||
| if (!exportDirectoryName) { | ||
| return true; | ||
| } |
arsalansufi
force-pushed
the
arsalan/cvr-testing
branch
from
82fb427 to
2eae2f4
Compare
Since we might instantiate both a MockCentralScannerStore and a MockPrecinctScannerStore
arsalansufi
force-pushed
the
arsalan/cvr-testing
branch
from
2eae2f4 to
35505c9
Compare
Easiest reviewed by commit
Overview
Issue link: #4058
Finally adding the tests that I've punted on for so so long. This PR gets the new CVR export functions in
libs/backendup to 💯% code coverage. I'll be opening a separate PR for the new CVR import functions.