Introduce prismock and use it in transaction agent spec #786
Conversation
skosito
requested review from
skarred14,
ognjenkurtic,
Therecanbeonlyone1969,
Kasshern,
Ybittan and
biscuitdey
as code owners
| import { AuthAgent } from '../../auth/agent/auth.agent'; | ||
| import { BpiSubjectAccount } from '../../identity/bpiSubjectAccounts/models/bpiSubjectAccount'; | ||
| import { BpiSubject } from '../../identity/bpiSubjects/models/bpiSubject'; | ||
| import { BpiSubjectAccount as ModelBpiSubjectAccount } from '../../identity/bpiSubjectAccounts/models/bpiSubjectAccount'; |
There was a problem hiding this comment.
it is the other way around in the prisma mapper when it comes to naming.
Domain objects imported without aliases.
PrismaModels have PrismaModel suffix.
Could we do the same here and use this as a convention throughout the codebase?
We can also turn it around, i do not care as long as we are consistent
| // and implement various test data scenarios that can be selected with a single line of code. | ||
| // https://github.com/demonsters/prisma-mock | ||
| const existingWorkgroupId = uuid(); | ||
| bpiSubject2 = await prisma.bpiSubject.create({ |
There was a problem hiding this comment.
we have a lot of builiders implemented in builders.ts.
Could we can use them to define the test domain objects we want to work with and then just have separate methods that abstract away prisma.create calls? This way we can share those methods across the test code base.
This way we could probably ditch the as ModelBpiSubjectAccount syntax as well.
Convetion would be that we work with domain objects in tests, and prisma create calls are only there to avoid mocking the storage agents
There was a problem hiding this comment.
what is advantage of using those builders compared to just calling constructors? seems like a redundant wrapper and one more place to maintain same thing as domain object constructor, every time new field is added, prisma schema, domain object and builder have to be updated
not sure i understand the point here, is this the flow:
- create object using builder
- then use that object to call prisma create (prisma create doesnt accept domain object as argument, we would need to map it or something like that)
- then use that same domain object again in tests?
i think these objects are mutable so using them around seems a bit weird, wouldn't it be simpler that every spec file sets up its own data? but no strong opinion here, i can modify if the flow i described above is what you meant
There was a problem hiding this comment.
It is mainly for readability and maintainability purposes. I agree it might be too much at this point. Maybe just open an issue and we can introduce them later if time allows.
There was a problem hiding this comment.
ok maybe lets merge this PR and i will apply this to all tests in follow up PR and we can see then what makes sense to be abstracted in builders etc? maybe we can also clean up builders after all tests are adapted
There was a problem hiding this comment.
@skosito can you open an issue for this so we can keep track of it? cc @ognjenkurtic @Kasshern
There was a problem hiding this comment.
@skosito dont forget to open an issue for this one pls.
| '123', | ||
| fromBpiSubjectAccount, | ||
| undefined as unknown as BpiSubjectAccount, | ||
| workflow.id + '', |
There was a problem hiding this comment.
hmm not sure, but its not needed
| workstepStorageAgentMock.getWorkstepById.mockResolvedValueOnce( | ||
| existingWorkstep1, | ||
| ); | ||
|
|
||
| const tx = new Transaction( |
There was a problem hiding this comment.
could you add transaction builder as well?
There was a problem hiding this comment.
Modulo the builder comment from @ognjenkurtic which will be addressed in a separate PT I am good
Description
Adds prismock library. I used this instead of prisma-mock library which is mentioned in issue because it seems maintained more often, and they are doing the same thing anyways.
Reworks transaction agent specs to use this library and fix some tests there. NOTE: make sure to expand diff in transaction agent spec since it's hidden in diff by default.
Bumps nodejs version to
17.0.0in workflow because prisma needs it.Cleanup of
NOT_FOUNDmessages in workstep and workflow modules.Once approach is approved I will modify all unit tests in the repo in follow up PR.
Related Issue
#742
Motivation and Context
Better way to write unit tests.
How Has This Been Tested
This modifies tests only, so it is tested by default.
Screenshots (if appropriate)
Types of changes
Checklist