Add test cases and update dashboards code to account for generic path #22
Conversation
johnnyon-amzn
changed the title
|
didn't pass DCO check i think it need to signoff commit message |
__tests__/dashboard.test.js
Outdated
| @@ -0,0 +1,29 @@ | |||
| const path = require('path'); | |||
There was a problem hiding this comment.
could we group all type of tests (UT, IT, FRT) into test folder like src folder
There was a problem hiding this comment.
Does the __tests__ folder count?
There was a problem hiding this comment.
__tests__ name looks verify specific to jest , could we be more generic
https://stackoverflow.com/questions/55551350/jest-test-formatting-tests-vs-test-js
-
unit test might put together with original code
e.g.
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/core_app/core_app.test.ts -
other test like functional test could put on test folder. e.g.
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/test/functional/apps/dashboard/index.js
https://github.com/microsoft/vscode/tree/main/test
what do you think?
src/dashboards.js
Outdated
|
|
||
|
|
||
| function getConfig() { | ||
| function getConfig(CONFIG_PATH=CONFIG_PATH) { |
There was a problem hiding this comment.
change variable name to configPath to avoid confusion
There was a problem hiding this comment.
The JS closure concept, it is trick to tell CONFIG_PATH is local variable, method variable or global variable to non-js developer.
let's use clear variable name for each scope to avoid that.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures#closure
__tests__/dashboard.test.js
Outdated
| @@ -0,0 +1,29 @@ | |||
| const path = require('path'); | |||
There was a problem hiding this comment.
__tests__ name looks verify specific to jest , could we be more generic
https://stackoverflow.com/questions/55551350/jest-test-formatting-tests-vs-test-js
-
unit test might put together with original code
e.g.
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/core_app/core_app.test.ts -
other test like functional test could put on test folder. e.g.
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/test/functional/apps/dashboard/index.js
https://github.com/microsoft/vscode/tree/main/test
what do you think?
src/dashboards.js
Outdated
| config[key] = value; | ||
| fs.writeFile(CONFIG_PATH, JSON.stringify(config), callback); | ||
| fs.writeFile(config_path, JSON.stringify(config), callback); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Updated in 9642596
src/dashboards.js
Outdated
| @@ -13,14 +13,24 @@ const superagent = require('superagent'); | |||
| const CONFIG_PATH = path.join(__dirname, "config.json"); | |||
|
|
|||
|
|
|||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
I was trying to rebase and messed up. I hard reset to the previous commit I had
|
DCO check seems, failed again |
Signed-off-by: Johnny On <[email protected]>
Signed-off-by: Johnny On <[email protected]>
Signed-off-by: Johnny On <[email protected]>
Signed-off-by: Johnny On <[email protected]>
Signed-off-by: Johnny On <[email protected]>
Description
Initial start of adding test cases to project.
Issues Resolved
#18
Test cases