Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unit tests for api-requests modules #44

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Add unit tests for api-requests modules #44

merged 1 commit into from
Sep 27, 2023

Conversation

Siegrift
Copy link
Collaborator

Closes #25

  • During the test implementation I noticed that some logs that were a bit misleading so I improved them.
  • There were some unnecessary types and wrong typings which I also corrected.
  • Since I made the above changes, pay a bit more attention to production files (not ending with .test.ts and not inside test folder)
  • There are some other changes, explained inside the PR.

@Siegrift Siegrift self-assigned this Sep 26, 2023
@Siegrift Siegrift requested a review from andreogle September 26, 2023 13:07
it('makes a single template request for multiple beacons', async () => {
const state = stateModule.getInitialState(config);
jest.spyOn(stateModule, 'getState').mockReturnValue(state);
const logger = createMockedLogger();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: If you want to know what is actually logged, you can do:

(logger as any).debug.mockImplementation(console.log)

which will forward the logs to console log. I used that a bit.


export const callApi = async (payload: node.ApiCallPayload) => {
getLogger().debug('Preprocessing API call payload', { aggregateApiCall: payload.aggregatedApiCall });
getLogger().debug('Preprocessing API call payload', pick(payload.aggregatedApiCall, ['endpointName', 'oisTitle']));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example file is uses pre/post processing to only make a single API call and if you log the full aggregateApiCall you also log parameters (of a single beacon) which is confusing, so I only log endpointName and oisTitle.

const processedPayload = await preProcessApiSpecifications(payload);
getLogger().debug('Performing API call', { aggregateApiCall: payload.aggregatedApiCall });
getLogger().debug('Performing API call', { processedPayload: processedPayload });
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug before - we should have logged the processed payloed.

getLogger().info(`Pushed signed data updates to the pool.`, { ...logContext, count: goRes.data.count });

getLogger().debug('Parsing response from the data registry.', { ...logContext, axiosResponse: goAxiosRequest.data });
const parsedResponse = dataRegistryResponseSchema.safeParse(goAxiosRequest.data);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not handled correctly previously.

@@ -9,15 +9,13 @@ import { loadConfig } from './validation/config';
import { initiateFetchingBeaconData } from './fetch-beacon-data';
import { initiateUpdatingSignedApi } from './update-signed-api';
import { initializeState } from './state';
import { initializeWallet } from './wallets';
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke the tests, since the getInitialState function essentially returned invalid state (wallet was missing).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved all of the fixtures to this file. It makes the tests a bit cleaner and we might use these in case we want to make an e2e test later on.

@Siegrift Siegrift changed the title Add unit tests for api-requests modules WIP: Add unit tests for api-requests modules Sep 27, 2023
@Siegrift
Copy link
Collaborator Author

Siegrift commented Sep 27, 2023

@andreogle I anticipated we will do #45 and renamed some of the variables to data registry for clarity. I need to revert this back to the original naming (that's why I added the WIP).

EDIT: The above was done.

@Siegrift Siegrift force-pushed the unit-tests branch 2 times, most recently from 3fc60e6 to 3fecb9b Compare September 27, 2023 08:31
Base automatically changed from document-logger to main September 27, 2023 08:32
@Siegrift Siegrift changed the title WIP: Add unit tests for api-requests modules Add unit tests for api-requests modules Sep 27, 2023
@Siegrift Siegrift merged commit 9a9dbaf into main Sep 27, 2023
3 checks passed
@Siegrift Siegrift deleted the unit-tests branch September 27, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plan and implement unit tests
2 participants