AN-297-Adding unit tests for Device Registry. #1343
Conversation
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Thanks @BenjaminSsempala , I have just left some comments for your review. Thanks!
| @@ -1,3 +1,4 @@ | |||
| require("module-alias/register"); | |||
There was a problem hiding this comment.
What was the motivation for adding the "alias" module to this Models file? Does it help out in the testing process? @BenjaminSsempala
There was a problem hiding this comment.
@Baalmart yes, I discovered it was required but I guess because I shifted focus to first finish utils, I'll look more into this as I go into models
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Thank you @BenjaminSsempala
This is really great work 👏👏👏
I have however just left a few comments that I think could be helpful in refactoring.
Thanks again
| let isValid = siteUtil.validateSiteName("134141341"); | ||
| assert.equal(isValid, true, "the site name is longer than 4 characters"); | ||
| it("should return true if the site name is not longer than 50 characters and shorter than 4 characters", function () { | ||
| let isValid = siteUtil.checkStringLength(stubValue.name); |
There was a problem hiding this comment.
What is the value of stubValue.name?
The test definition states that its length is
4 < stubValue.name < 51
However, I can't tell which edge case is being tested here.
Could we test both cases individually in this case for example
Have stubValue.name with a length less than 4, a value in the acceptable range, and a value greater than 50?
| // it("should generate a unique site name given the parish name", function () { | ||
|
|
||
| // const stub = sinon | ||
| // .stub(siteModel(stubValue.tenant), "create") | ||
| // .returns(stubValue); |
There was a problem hiding this comment.
I am a strong proponent of removing unused code rather than just commenting it out. This is because this code will always be available in the git history should you ever need it in the future.
However, this is not a big issue for now so no worries
| { | ||
| params: { | ||
| locations: [{ lat: stubValue.latitude, lng: stubValue.longitude }], | ||
| key: process.env.GOOGLE_MAPS_API_KEY, |
There was a problem hiding this comment.
Is it possible to mock the Google maps api response and avoid passing in the actual key.
With this implementation, I can tell we are sending the test requests to google maps which may not be necessary cost wise
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let match; | ||
| while ((match = regex.exec(fileContent))) { | ||
| const testName = match[1]; | ||
| const testCommand = `mocha ${filePath} --grep "${testName.replace(/"/g, '\\"')}" --exit`; |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Hi @BenjaminSsempala , is this PR still relevant? |
WHAT DOES THIS PR DO?
STATUS OF CODE COMPLETION
Utils
STATUS OF REVIEW
WHAT ISSUES ARE RELATED TO THIS PR?
HOW DO I TEST OUT THIS PR?
mocha ./utils/test/ut_create-site.js --exit-You can currently test out the util tests depending on their indicated status of completion above. Simply replace the
ut_create-site.jsin the command below with the targeted test-file for the specific test, for-exampleut_create-device.jsWHICH ENDPOINTS SHOULD BE READY FOR TESTING?:
IS THERE ANY JENKINS CONSOLE LINK FOR CODE COVERAGE AND BUILD INFO?
ARE THERE ANY RELATED PRs?