Refactored the OpenMRS test suite #963
Conversation
There was a problem hiding this comment.
Hi @PiusKariuki, thank you for this.
I'm really sorry but I'm not sold on some of the changes. As a rule I would prefer tests to be verbose but readable rather than super efficient. I think some of these refactors make the tests harder to read and understand and extend. I also think we've lost some important test logic?
I don't want to spend a lot of time on this going forward. If you can quickly and easily keep some of the smaller changes - the state and data re-use for example - then do it. Otherwise I think I have to close this down. No more than an hour of dev from this point please.
packages/openmrs/test/index.test.js
Outdated
| @@ -66,65 +66,43 @@ describe('execute', () => { | |||
| }); | |||
|
|
|||
| describe('http', () => { | |||
| it('should GET with a query', async () => { | |||
| beforeEach(()=> { | |||
There was a problem hiding this comment.
hmm, I think there are two problems with this structure 🤔
- The interceptor is quite far removed from the test. Like, physically. When I'm looking at the test, it's hard to understand how the mock endpoint will behave.
- Each of these interceptors only triggers once. So they're literally a 1;1 binding to the test that uses them. And again, they're quite far removed. You could use
.persist()to make the interceptor permanent (until we tear it down). But that's only helpful if we have multiple tests using the same endpoint.
So I'm really sorry but I'm not convinced that this is a positive change.
If I was to radically think about how to re-do these unit tests (and I'm really not sure I want to invest time in this), I would probably build a mock server and add an API to add data to it. So rather than mocking out very specific endpoints, I could just add or remove data to the server to get the right behaviour out of it. But I do think that's way too much work for this adaptor for now.
|
|
||
| it('should auto-fetch patients with a limit', async () => { |
There was a problem hiding this comment.
Have we dropped this test entirely? Why? It's testing some quite important behaviour
packages/openmrs/test/index.test.js
Outdated
| query: { q: 'Sarah', limit: 1 }, | ||
| baseUrl: state.configuration.instanceUrl, | ||
| }); | ||
| .reply(200, { results: testData.patientResults }, { ...jsonHeaders }); |
There was a problem hiding this comment.
I do approve of re-using the same test data - that makes the tests much more concise and readable
packages/openmrs/test/index.test.js
Outdated
| { ...jsonHeaders } | ||
| ); | ||
| // Define state only once | ||
| const state = { configuration } |
There was a problem hiding this comment.
Yep - we could probably do this once at the top of the file couldn't we? Because actually, despite what this comment says, by my count we define state 14 times 😅
There was a problem hiding this comment.
Yeah you're right
| }); | ||
|
|
||
| it('should not auto-fetch if the user sets startIdex', async () => { | ||
| testServer | ||
| .intercept({ |
There was a problem hiding this comment.
I'm concerned we've lost some important test logic here too
|
Hi @josephjclark I totally understand this let me wrap this up quickly. I will revert the commit and just keep the state and data re-use logic. |
Summary
Refactor the OpenMRS test suite
Fixes #
Details
Removing duplicates and storing interceptors in beforeEach blocks
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to
know!):
You can read more details in our
Responsible AI Policy
Review Checklist
Before merging, the reviewer should check the following items:
production? Is it safe to release?
dev only changes don't need a changeset.