refactor(blobstorage): tests converted to typescript #3742
Conversation
| */ | ||
| export async function createTestStream( | ||
| streamObj: BasicTestStream, | ||
| export async function createTestStream( //FIXME this function has side-effects, and amends the object in-place |
There was a problem hiding this comment.
@iainsproat The side-effects are definitely intentional
There was a problem hiding this comment.
If/when we start running tests in parallel, the side-effects have a higher risk of test pollution if we're not super careful.
There was a problem hiding this comment.
well, this aspect of the function is a part of its core responsibilities, its pretty important to have it cause manually having to import id-generating functions, having to remember the correct lengths & formats of ids is a PITA that this aspect of the helper resolves. in fact, this was one of the main reasons for me even creating these. it would be a serious impact to DX if we were to remove it.
its not immediately obvious to me how these helpers would cause issues in parallel tests, but if any pop up we can deal with them then
There was a problem hiding this comment.
The helper is fine, but it should ideally treat input as immutable. I'd expect it to return a deep clone plus the id as its output.
| const createStreamForTest = async () => { | ||
| const stream = { | ||
| const createStreamForTest = async (streamOwner: BasicTestUser) => { | ||
| const stream: Partial<BasicTestStream> = { |
There was a problem hiding this comment.
Isn't it always the same user?
There was a problem hiding this comment.
It makes it explicit which user is being used for the test. I'd like to get to the point where we can run more tests in parallel, or at least shuffle the order, and making test object dependencies clear is going to be helpful for avoiding test pollution.
So yes, it's always the same user - at least for now.
Description & motivation
Typescripting more of the things.
Changes:
createStreamForTestfunction. This reduces surprising side-effects (in the previous case, the global user being used as the owner).To-do before merge:
Screenshots:
Validation of changes:
Checklist:
References