-
Notifications
You must be signed in to change notification settings - Fork 7
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
chore(tests): [RO-26598] Improve test coverage #83
Changes from all commits
ebe0638
4d1c150
b4011f4
b916e19
99e21c3
61c193b
61c62d7
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,12 +1,14 @@ | ||
root = true | ||
|
||
[*] | ||
indent_style = space | ||
indent_size = 2 | ||
end_of_line = lf | ||
charset = utf-8 | ||
trim_trailing_whitespace = true | ||
end_of_line = lf | ||
indent_size = 2 | ||
indent_style = space | ||
insert_final_newline = true | ||
max_line_length = 120 | ||
quote_type = single | ||
trim_trailing_whitespace = true | ||
|
||
[node_modules/**.js] | ||
codepaint = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"prettier.enable": false, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,8 @@ | ||
const _some = require('lodash.some'); | ||
const _assignIn = require('lodash.assignin'); | ||
|
||
module.exports = function ( | ||
SpurErrors, | ||
Logger, | ||
HtmlErrorRender, | ||
BaseMiddleware | ||
) { | ||
module.exports = function (SpurErrors, Logger, HtmlErrorRender, BaseMiddleware) { | ||
class ErrorMiddleware extends BaseMiddleware { | ||
|
||
configure(app) { | ||
super.configure(app); | ||
|
||
|
@@ -34,29 +28,29 @@ module.exports = function ( | |
res.format({ | ||
text: () => this.sendTextResponse(err, req, res), | ||
html: () => this.sendHtmlResponse(err, req, res), | ||
json: () => this.sendJsonResponse(err, req, res) | ||
json: () => this.sendJsonResponse(err, req, res), | ||
}); | ||
|
||
next(); | ||
} | ||
|
||
logErrorStack(err) { | ||
logErrorStack(error) { | ||
const err = error ?? {}; | ||
const statusCode = err.statusCode || 0; | ||
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. Assigning fallback According to the unit test coverage, the 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. Weird. I thought this was added as a patch. I don't recall why but if you add this I can't guarantee you may not introduce a big issue. Remember this is dependency injected, so there are callers outside of this code base that could trigger this, so I would refrain from refactoring code checks that are not needed. The intent of this project is to remove dependecies, not refactoring 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. I don't follow, when using the middleware as expected, When calling the method directly, e.g. I will revert the change, but it defies logic |
||
const checkStatus = (status) => status === statusCode; | ||
|
||
if (!_some(this.EXCLUDE_STATUSCODE_FROM_LOGS, checkStatus)) { | ||
Logger.error(err, '\n', err.stack, '\n', (err.data || '')); | ||
Logger.error(err, '\n', err?.stack ?? '', '\n', err?.data ?? ''); | ||
} | ||
} | ||
|
||
appendRequestData(err, req) { | ||
if (err.data == null) { | ||
err.data = {}; | ||
} | ||
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. I think this was a patch to something. The fact that it wasn't defaulted to an empty object. 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. |
||
|
||
err.data = _assignIn(err.data, { | ||
url: req.url | ||
}); | ||
err.data = _assignIn( | ||
{ ...err.data }, | ||
{ | ||
url: req.url, | ||
}, | ||
); | ||
} | ||
|
||
sendTextResponse(err, req, res) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,5 @@ | ||
module.exports = function ( | ||
Promise, | ||
BaseMiddleware | ||
) { | ||
|
||
module.exports = function (Promise, BaseMiddleware) { | ||
class PromiseMiddleware extends BaseMiddleware { | ||
|
||
configure(app) { | ||
super.configure(app); | ||
|
||
|
@@ -14,8 +9,8 @@ module.exports = function ( | |
.catch(this.req.next); | ||
}; | ||
|
||
this.app.response.renderAsync = function (view, properties = {}) { | ||
return Promise.props(properties) | ||
this.app.response.renderAsync = function (view, properties) { | ||
Comment on lines
-17
to
-18
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. Moved the "default" value of |
||
return Promise.props(properties ?? {}) | ||
.then((props) => this.render(view, props)) | ||
.catch(this.req.next); | ||
}; | ||
|
@@ -42,7 +37,7 @@ module.exports = function ( | |
}, | ||
json: () => { | ||
this.json(results); | ||
} | ||
}, | ||
}); | ||
}) | ||
.catch(this.req.next); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,22 +7,15 @@ module.exports = function ( | |
ErrorMiddleware, | ||
config, | ||
ControllerRegistration, | ||
WinstonRequestLoggingMiddleware | ||
WinstonRequestLoggingMiddleware, | ||
) { | ||
class BaseWebServer { | ||
|
||
constructor() { | ||
this.app = express(); | ||
} | ||
|
||
getPort() { | ||
let port = config.Port; | ||
|
||
if (this.server) { | ||
port = this.server.address().port || port; | ||
} | ||
|
||
return port; | ||
Comment on lines
-19
to
-25
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. Simplified logic because |
||
return this.server?.address()?.port ?? config.Port; | ||
} | ||
|
||
registerDefaultMiddleware() { | ||
|
@@ -88,19 +81,20 @@ module.exports = function ( | |
} | ||
|
||
getCloseAsync() { | ||
if (this.server && this.server.closeAsync) { | ||
if (this.server?.closeAsync) { | ||
return this.server.closeAsync(); | ||
} | ||
|
||
return Promise.resolve(); | ||
} | ||
|
||
startedMessage() { | ||
const port = this.getPort(); | ||
if (this.cluster) { | ||
return `Worker ${this.cluster.worker.id} started on port ${this.getPort()}`; | ||
return `Worker ${this.cluster.worker.id} started on port ${port}`; | ||
} | ||
|
||
return `Express app started on port ${this.getPort()}`; | ||
return `Express app started on port ${port}`; | ||
} | ||
|
||
logSectionHeader(message) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,24 @@ | ||
module.exports = function (BaseController, Promise) { | ||
const fs = require('fs'); | ||
|
||
module.exports = function (BaseController, Promise) { | ||
class PromiseMiddlewareTestController extends BaseController { | ||
|
||
configure(app) { | ||
super.configure(app); | ||
|
||
const mockOpenTableTemplateViewEngine = (filePath, options, callback) => { | ||
fs.readFile(filePath, (err, content) => { | ||
const { settings, _locals, cache, ...vars } = options; | ||
let rendered = content.toString(); | ||
Object.entries(vars).forEach(([placeholder, value]) => { | ||
rendered = rendered.replace(`{{${placeholder}}}`, value); | ||
}); | ||
return callback(null, rendered); | ||
}); | ||
}; | ||
|
||
app.engine('ott', mockOpenTableTemplateViewEngine); | ||
app.set('views', `${__dirname}/views`); | ||
|
||
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. This is not needed. In the test you mock and inject ejs, where ever it's failing should be depending on PromiseMiddlewareTestController You will need code like this in near the existing injector:
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.
From my experience trying to implement your injection suggestion, |
||
app.get('/promise-middleware-test--jsonasync', this.getJsonAsync); | ||
app.get('/promise-middleware-test--renderasync', this.getRenderAsync); | ||
app.get('/promise-middleware-test--sendAsync', this.getSendAsync); | ||
|
@@ -16,7 +31,15 @@ module.exports = function (BaseController, Promise) { | |
} | ||
|
||
getRenderAsync(req, res) { | ||
res.renderAsync('renderView', Promise.resolve({ microapp: 'renderAsync success' })); | ||
const { headers } = req; | ||
const view = headers['x-view']; | ||
const viewPropsHeader = headers['x-view-props']; | ||
const viewProps = viewPropsHeader !== 'undefined' ? JSON.parse(viewPropsHeader) : undefined; | ||
if (viewProps) { | ||
res.renderAsync(view, Promise.resolve(viewProps)); | ||
} else { | ||
res.renderAsync(view); | ||
} | ||
} | ||
|
||
getSendAsync(req, res) { | ||
|
@@ -28,7 +51,7 @@ module.exports = function (BaseController, Promise) { | |
} | ||
|
||
getFormatAsync(req, res) { | ||
res.formatAsync(Promise.resolve('formatAsync success')); | ||
res.formatAsync('innerHTML', Promise.resolve('formatAsync success')); | ||
} | ||
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. Not sure why this is needed. 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. Just a dummy view to test the view-engine, it's required event with a mocked view engine |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<title>Testing</title> | ||
</head> | ||
<body> | ||
No render view-props provided | ||
</body> | ||
</html> |
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. Just a dummy view to test the view-engine, it's required event with a mocked view engine |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<title>Testing</title> | ||
</head> | ||
<body> | ||
{{microapp}} | ||
</body> | ||
</html> |
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 is this for? And why was it working beore? what expectation does it change?
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.
This just adds the name of the device to the
request.device
object. Previously it would returntype: 'phone', name: ''
, with this change it will returntype: 'phone', name: 'iPhone'
. Might be good for logging/stats or other reasons.