[keyserver] Update responder-validators.test.js to include specialRole field
AbandonedPublic
Actions

Authored by atul on Nov 30 2023, 1:13 PM.

Details

Reviewers

ginsu
tomek
rohan

Summary

There are some places in the codebase that we hardcode isDefault. Here, we'll want to introduce the specialRole field alongside, so when we deprecate isDefault, no additional changes will be needed. Some are important (like one instance in thread-utils.js and the instance in role-creator.js where we set up initial roles for threads), and others are just test data. Nonetheless, I thought I may as well update all of them for consistency.

Part of ENG-5993

This diff handles responder-validators.test.js

Depends on D10109

Test Plan

Ran yarn workspace keyserver test to make sure unit tests still passed

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Nov 30 2023, 1:13 PM

rohan held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptNov 30 2023, 1:13 PM

rohan added a child revision: D10111: [lib] Update minimally-encoded-thread-permissions.test.js to include specialRole field.Nov 30 2023, 1:13 PM

rohan edited the test plan for this revision. (Show Details)Nov 30 2023, 1:27 PM

rohan published this revision for review.Nov 30 2023, 1:33 PM

Harbormaster failed remote builds in B24656: Diff 34069!Nov 30 2023, 2:19 PM

Harbormaster completed remote builds in B24656: Diff 34069.Nov 30 2023, 4:39 PM

ashoat added inline comments.Nov 30 2023, 6:16 PM

keyserver/src/responders/responder-validators.test.js
132 ↗	(On Diff #34069)	Constant

rohan added inline comments.Dec 1 2023, 6:11 AM

keyserver/src/responders/responder-validators.test.js
132 ↗	(On Diff #34069)	I felt like a constant like `specialRoles.DEFAULT_ROLE` wasn't best here (and in the other places D10112, D10113, D10114, D10115) because all of this test data is what the `threadStore` looks like when you console.log it / copy it from redux / etc. So all of the values are essentially 'hardcoded' here. Ultimately though it's the same either way so I'm ok to change it if you disagree

ashoat added inline comments.Dec 1 2023, 8:58 AM

keyserver/src/responders/responder-validators.test.js
132 ↗	(On Diff #34069)	I don't feel super strongly, but I think a constant is better. @atul @ginsu @tomek what do you think?

atul added inline comments.Dec 1 2023, 9:05 AM

keyserver/src/responders/responder-validators.test.js
132 ↗	(On Diff #34069)	I can see both sides. I've definitely had to go from literal => constant when pasting in from Redux which is annoying, but constant is more readable/understandable for anyone looking at code in future. Not sure if this applies to test code, but given that generally "people read code more than they write code," I'd lean on replacing literals with constant.
132 ↗	(On Diff #34069)	*lean towards