(test) Remove unnecessary partial mocks; improve typing in tests #1933
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
denniskigen
requested review from
ibacher,
jayasanka-sack,
brandones and
NethmiRodrigo
|
Size Change: -56 B (0%) Total Size: 11.1 MB ℹ️ View Unchanged
|
brandones
approved these changes
Jul 23, 2024
There was a problem hiding this comment.
LGTM, thanks @denniskigen .
I wonder if we can make a nice pattern that ensures that typescript or the test breaks if mockUseConfig.mockReturnValue is set to something incompatible with the config type. Maybe like
mockUseConfig.mockReturnValue({
concepts: {
pulseUuid: '5087AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
},
} as Partial<ConfigObject>);
?
|
@brandones if we want strict conformance with the source function's type definitions, we can do something like: import type { ConfigObject } from './config-schema';
const mockUseConfig = jest.mocked<() => ConfigObject>(useConfig);And then by default, TypeScript will expect you to provide an object that fully satisfies the contract as the mock's return value: beforeEach(() => {
mockUseConfig.mockReturnValue({
biometrics: { bmiUnit: 'kg / m²', heightUnit: 'm', weightUnit: 'kg' },
concepts: {
diastolicBloodPressureUuid: '5086AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
heightUuid: '5090AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
midUpperArmCircumferenceUuid: '1343AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
oxygenSaturationUuid: '5092AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
pulseUuid: '5087AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
respiratoryRateUuid: '5242AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
systolicBloodPressureUuid: '5085AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
temperatureUuid: '5088AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
weightUuid: '5089AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',
},
vitals: {
encounterTypeUuid: '67a71486-1a54-468f-ac3e-7091a9a79584',
formUuid: 'a000cb34-9ec1-4344-a1c8-f692232f6edd',
useFormEngine: false,
useMuacColors: false,
formName: 'Vitals',
showPrintButton: false,
},
});
}); If we want to satisfy the type contract with a partial config, we could then do something like: beforeEach(() => {
mockUseConfig.mockReturnValue({
concepts: {
pulseUuid: false,
},
} as ConfigObject);
});This will fail type checking because the |
|
Yeah I think the latter is reasonable. I wonder if it also works well with beforeEach(() => {
mockUseConfig.mockReturnValue({
...getDefaultsFromConfigSchema(schema),
concepts: {
pulseUuid: "asdfasdfasdf",
},
} as ConfigObject);
});which IMO may be the best way to do config overrides |
|
It does. I'd forgotten about
The added benefit is that you don't need to typecast your partial config mock using |
denniskigen
force-pushed
the
test/remove-unnecessary-partial-mocks
branch
2 times, most recently
from
4e6276e
to
cb434e8
Compare
brandones
reviewed
Jul 23, 2024
| @@ -38,9 +44,13 @@ jest.mock('../common', () => ({ | |||
| })), | |||
| })); | |||
|
|
|||
| mockUseConfig.mockReturnValue({ | |||
| ...getDefaultsFromConfigSchema(configSchema), | |||
| mockVitalsConfig, | |||
There was a problem hiding this comment.
This seems like it would create a key called mockVitalsConfig on the object, which I would really hope would produce a type error
There was a problem hiding this comment.
Fixed in the latest push, I think.
denniskigen
force-pushed
the
test/remove-unnecessary-partial-mocks
branch
2 times, most recently
from
8b49498
to
02f59d6
Compare
denniskigen
force-pushed
the
test/remove-unnecessary-partial-mocks
branch
from
02f59d6
to
edac26d
Compare
|
I'm sorry for blowing the blast radius of this PR a fair bit after review. The main motivators are:
|
brandones
reviewed
Jul 24, 2024
| @@ -1,42 +1,91 @@ | |||
| import { Type } from '@openmrs/esm-framework'; | |||
|
|
|||
| export const esmPatientChartSchema = { | |||
| defaultFacilityUrl: { | |||
brandones
approved these changes
Jul 24, 2024
brandones
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Summary
This PR refactors various tests in the Patient Chart to fix the issue of unnecessary partial mocks. The mock.tsx manual mock in Core aggregates stubs for most functions in Core. We should only extend the manual mock if we want to include stubs for things that exist in Core, but are not included in the manual mock. Otherwise, we should extend the stubs already provided by the manual mock. This PR enables just that.
Screenshots
Related Issue
Other
Documentation updates will follow shortly.