Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Unit test controller and service of each core Module#75

Merged
LeonFLK merged 20 commits intodevelopfrom
lw-unit-tests
Sep 4, 2020
Merged

Unit test controller and service of each core Module#75
LeonFLK merged 20 commits intodevelopfrom
lw-unit-tests

Conversation

@LeonFLK
Copy link
Contributor

@LeonFLK LeonFLK commented Aug 28, 2020

fixes KILTProtocol/ticket#616

##fixes KILTProtocol/ticket#721
##fixes KILTProtocol/ticket#720
##fixes KILTProtocol/ticket#718
##fixes KILTProtocol/ticket#686
##fixes KILTProtocol/ticket#724
##fixes KILTProtocol/ticket#730

  • CTypes module
  • Messaging Module
  • Contacts Module
  • Faucet Module

This introduces unit testing of controller and service for each core module, via faking the modules service and mocking of the mongoose Models.
Removes naive approach of in-memory unit testing.
Also aims to refactor some minor issues within our classes.

Contact Module changes:
Removed _findByAddress, and replaced calling it with directly querying the DB.
converToContact is now a private function inside MongoDbMContactsService.

CType Module changes:
Renamed verifyCType to verifyCTypeAndReturnChainOwner, to more accurately represent the updated behaviour. Please suggest a better name!
Removed _findByHash, replaced calls with directly querying DB.
Changed convertToCType from exported to private.

Faucet Module Changes:
Removed email from POST payload parameter.
Removed try catch to check address in favor of checkAddress call.
Now only accepts actual addresses and NOT public keys, or anthing else, prefix of address has to match KILT prefix of 42.

Messaging Module Changes:
Changed uuidv4() from unexported function to exported constant.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@LeonFLK LeonFLK self-assigned this Aug 28, 2020
@LeonFLK LeonFLK changed the title test: messaging,ctypes,contacts module tests and fixes 721,718 Unit test controller and service of each core Module Aug 28, 2020
@LeonFLK LeonFLK requested review from rflechtner and tjwelde August 28, 2020 13:03
@LeonFLK LeonFLK marked this pull request as ready for review September 2, 2020 12:23
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Be sure to await any expect(async() => {}).resolves and expect(async() => {}).rejects like this:
await expect(async() => true).resolves.toBeTruthy()
Other than that I have no functional complaints, the remaining comments are all style-related. To summarise, there's two things that could help readability of the tests:

  1. Choose one way to mock async functions and to return promises. My personal favourite is async(): Promise<any> => value, i.e. async(): Promise<void> => {}. (): Promise<any> => Promise.resolve(value) also works, but async (): Promise<any> => Promise.resolve(value) seems a bit redundant
  2. Separate negative and positive tests. In general its nicer not to lump too many different cases in one it(), but checking happy path & correct error handling should definitely be two tests imo

Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

the failing integration tests indicate that this broke the API by requiring that a KILT address be sent to the faucet endpoint. You basically have two options to fix this:

  1. translate a public key to an address before running it through the address validator. In this case it would be cool to amend integration tests so that we call the faucet endpoint with both an address and a public key. Just add an extra test for address.
  2. change the api. This would of course require changing the integration tests, but most importantly we'd have to change the pubkey key in the payload to address; imho we can't ask for a public key but secretly require an address, even in a non-public api. This would mean changing the faucet implementation as well.

@LeonFLK
Copy link
Contributor Author

LeonFLK commented Sep 4, 2020

Good points, and yes you are requesting with a pubkey in the Integration tests. I'll talk to TImo, what the services should be capable of. But I think the best solution atm for this would be to convert to address before checking validity and eligibility.

@LeonFLK
Copy link
Contributor Author

LeonFLK commented Sep 4, 2020

Wir wollen nur die address erwarten und akzeptieren.
Der API key wird auch umbenannt, wird aber nur Auswirkungen aufs faucet haben.

@LeonFLK LeonFLK requested a review from rflechtner September 4, 2020 14:06
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Nice! There's an error message to fix (see my comment) but then you're good to go!

@LeonFLK LeonFLK merged commit 8d8a280 into develop Sep 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants