-
Notifications
You must be signed in to change notification settings - Fork 22
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
[INTERNAL] test: improved test coverage #91
Changes from 17 commits
ff42d8e
4d10beb
f96d4f9
9849ca1
38c2333
f32bd87
3105a90
32d2a0d
f82fd31
b266457
902af47
a508006
a2785bf
4fc780e
b4fef79
29fc902
c2b125f
30dd0b7
e129981
bfca920
bff7df0
9a92681
4e515b6
10012e8
9e8beb3
54ed49e
2ce5ac2
ea8f4de
6171838
a95d063
6963558
0832adc
93db6fe
2cf9b15
b4c6ddd
43055dc
95accb3
5072c11
41abd8c
feafead
b3eec89
10b711a
a3b97cc
014d995
bdc66e1
d0a4b38
bb3781b
0824ad1
1a78601
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
const ui5Cli = { | ||
init: require("./lib/init/init") | ||
init: require("./lib/init/init").init | ||
}; | ||
|
||
module.exports = ui5Cli; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,44 +1,34 @@ | ||
// Init | ||
const init = { | ||
const initCommand = { | ||
command: "init", | ||
describe: "Initialize the UI5 Build and Development Tooling configuration for an application or library project.", | ||
middlewares: [require("../middlewares/base.js")] | ||
}; | ||
|
||
init.handler = async function(argv) { | ||
const init = require("../../init/init"); | ||
const {promisify} = require("util"); | ||
const path = require("path"); | ||
const fs = require("fs"); | ||
const stat = promisify(fs.stat); | ||
const writeFile = promisify(fs.writeFile); | ||
const safeDumpYaml = require("js-yaml").safeDump; | ||
|
||
async function exists(filePath) { | ||
try { | ||
await stat(filePath); | ||
return true; | ||
} catch (err) { | ||
// "File or directory does not exist" | ||
if (err.code === "ENOENT") { | ||
return false; | ||
} else { | ||
throw err; | ||
} | ||
} | ||
} | ||
initCommand.lazyRequireDependencies = async function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the point of having this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, it's totally useless. Removed... |
||
return { | ||
fsHelper: require("../../utils/fsHelper"), | ||
init: require("../../init/init"), | ||
promisify: require("util").promisify, | ||
path: require("path"), | ||
fs: require("fs"), | ||
jsYaml: require("js-yaml") | ||
}; | ||
}; | ||
|
||
initCommand.handler = async function() { | ||
const {fsHelper, init, promisify, path, fs, jsYaml} = initCommand.lazyRequireDependencies(); | ||
const writeFile = promisify(fs.writeFile); | ||
try { | ||
const yamlPath = path.resolve("./ui5.yaml"); | ||
if (await exists(yamlPath)) { | ||
if (await fsHelper.exists(yamlPath)) { | ||
throw new Error("Initialization not possible: ui5.yaml already exists"); | ||
} | ||
|
||
const projectConfig = await init(); | ||
const yaml = safeDumpYaml(projectConfig); | ||
const projectConfig = await init.init(); | ||
const yaml = jsYaml.safeDump(projectConfig); | ||
|
||
await writeFile(yamlPath, yaml); | ||
|
||
console.log(`Wrote ui5.yaml to ${yamlPath}:\n`); | ||
console.log(yaml); | ||
} catch (err) { | ||
|
@@ -47,4 +37,4 @@ init.handler = async function(argv) { | |
} | ||
}; | ||
|
||
module.exports = init; | ||
module.exports = initCommand; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,8 +46,12 @@ serve.builder = function(cli) { | |
"Listen to port 1337 and launch default browser with http://localhost:1337/test/QUnit.html"); | ||
}; | ||
|
||
serve.handler = function(argv) { | ||
serve.openUrl = async function(browserUrl) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, adding a function just to make testing easier should be avoided if possible. What about mock-require? The function is called just one time, so no real need to have it in a separate function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How should we test the functionality of the url opening then? e.g. spy on opn call will not work.
|
||
const opn = require("opn"); | ||
return opn(browserUrl); | ||
}; | ||
|
||
serve.handler = function(argv = {}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the defaulting to empty object? argv should always be supplied shouldn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default param is there to avoid errors when properties are accessed e.g. normalizer.generateProjectTree({
translator: argv.translator,
configPath: argv.config
}) |
||
const normalizer = require("@ui5/project").normalizer; | ||
const ui5Server = require("@ui5/server"); | ||
const server = ui5Server.server; | ||
|
@@ -60,34 +64,39 @@ serve.handler = function(argv) { | |
port: argv.port === undefined ? argv.h2 ? 8443 : 8080 : argv.port, | ||
changePortIfInUse: argv.port === undefined, // only change if port isn't explicitly set | ||
h2: argv.h2, | ||
acceptRemoteConnections: argv["accept-remote-connections"] | ||
acceptRemoteConnections: !!argv.acceptRemoteConnections, | ||
cert: argv.h2 ? argv.cert : undefined, | ||
key: argv.h2 ? argv.key : undefined | ||
}; | ||
|
||
return Promise.resolve().then(function() { | ||
if (serverConfig.h2) { | ||
return ui5Server.sslUtil.getSslCertificate().then(({key, cert}) => { | ||
serverConfig.key = key; | ||
serverConfig.cert = cert; | ||
return serverConfig; | ||
}); | ||
} else { | ||
return serverConfig; | ||
} | ||
}).then((serverConfig) => { | ||
return server.serve(tree, serverConfig).then(function({h2, port}) { | ||
const protocol = h2 ? "https" : "http"; | ||
let browserUrl = protocol + "://localhost:" + port; | ||
if (argv.open !== undefined) { | ||
if (!serverConfig.h2) { | ||
return {serverConfig, tree}; | ||
} else { | ||
return ui5Server.sslUtil.getSslCertificate(serverConfig.key, serverConfig.cert).then(({key, cert}) => { | ||
serverConfig.key = key; | ||
serverConfig.cert = cert; | ||
return {serverConfig, tree}; | ||
}); | ||
} | ||
}).then(({serverConfig, tree}) => { | ||
return server.serve(tree, serverConfig).then(function({h2, port}) { | ||
const protocol = h2 ? "https" : "http"; | ||
let browserUrl = protocol + "://localhost:" + port; | ||
console.log("Server started"); | ||
console.log("URL: " + browserUrl); | ||
|
||
if (argv.open !== undefined) { | ||
if (typeof argv.open === "string") { | ||
let relPath = argv.open || "/"; | ||
if (!relPath.startsWith("/")) { | ||
relPath = "/" + relPath; | ||
} | ||
browserUrl += relPath; | ||
opn(browserUrl); | ||
} | ||
console.log("Server started"); | ||
console.log("URL: " + browserUrl); | ||
}); | ||
serve.openUrl(browserUrl); | ||
} | ||
|
||
return browserUrl; | ||
}); | ||
}).catch(function(err) { | ||
console.error(err); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,13 @@ | ||
const logger = require("./logger"); | ||
/** | ||
* Base middleware for CLI commands. | ||
* | ||
* This middleware should be executed for every CLI command to enable basic features (e.g. logging). | ||
* @param {Object} argv The CLI arguments | ||
* @returns {Object} Data to be appended to argv | ||
*/ | ||
module.exports = function(argv) { | ||
if (argv.verbose || argv.loglevel) { | ||
const logger = require("@ui5/logger"); | ||
if (argv.loglevel) { | ||
logger.setLevel(argv.loglevel); | ||
} | ||
if (argv.verbose) { | ||
logger.setLevel("verbose"); | ||
logger.getLogger("cli:middlewares:base").verbose(`using node version ${process.version}`); | ||
} | ||
} | ||
module.exports = function(argv = {}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default param is there to avoid errors when properties are accessed... |
||
if (Object.keys(argv).length === 0) return null; | ||
|
||
return {}; | ||
logger.init(argv); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/** | ||
* Logger middleware | ||
* | ||
* Logger middleware used as one of default middlewares by tooling | ||
* @param {Object} argv logger arguments | ||
* @returns {Object} logger instance or null | ||
*/ | ||
function init(argv = {}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default param is there to avoid errors when properties are accessed... |
||
if (!argv.verbose && !argv.loglevel) return null; | ||
|
||
const logger = require("@ui5/logger"); | ||
if (argv.loglevel) { | ||
logger.setLevel(argv.loglevel); | ||
} | ||
if (argv.verbose) { | ||
logger.setLevel("verbose"); | ||
logger.getLogger("cli:middlewares:base").verbose(`using node version ${process.version}`); | ||
} | ||
return logger; | ||
} | ||
|
||
module.exports = {init}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
const {promisify} = require("util"); | ||
const fs = require("fs"); | ||
const stat = promisify(fs.stat); | ||
const path = require("path"); | ||
|
||
/** | ||
* Checks if a file or path exists | ||
* | ||
* @private | ||
* @param {string} filePath Path to check | ||
* @returns {Promise} Promise resolving with true if the file or path exists | ||
*/ | ||
async function exists(filePath) { | ||
try { | ||
await stat(filePath); | ||
return true; | ||
} catch (err) { | ||
// "File or directory does not exist" | ||
if (err.code === "ENOENT") { | ||
return false; | ||
} else { | ||
throw err; | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Checks if a list of paths exists | ||
* | ||
* @private | ||
* @param {Array} paths List of paths to check | ||
* @param {string} cwd Current working directory | ||
* @returns {Promise} Resolving with an array of booleans for each path | ||
*/ | ||
async function pathsExist(paths, cwd) { | ||
return await Promise.all(paths.map((p) => exists(path.join(cwd, p)))); | ||
} | ||
|
||
module.exports = { | ||
exists, | ||
pathsExist | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using mock-require here instead of adding this function to the productive code just for resting purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to avoid adding a new dependency with a module which was maintained 9 months ago and "only" 300 stars + mocking module loaders is not really a good solution e.g. switching to ES6 modules will not be so easy anymore...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, module-require is now used to mock module loading