Skip to content
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

Merged
merged 7 commits into from
Apr 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions .editorconfig
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
3 changes: 3 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"prettier.enable": false,
}
12 changes: 6 additions & 6 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ module.exports = {
coveragePathIgnorePatterns: ['/node_modules/'],
coverageThreshold: {
global: {
branches: 62,
functions: 84,
lines: 89,
statements: 89
}
branches: 95,
functions: 98,
lines: 98,
statements: 98,
},
},
maxWorkers: 1,
moduleFileExtensions: ['js','json'],
moduleFileExtensions: ['js', 'json'],
rootDir: '.',
testEnvironment: 'node',
testMatch: ['**/*.spec.js'],
Expand Down
2 changes: 1 addition & 1 deletion src/middleware/DefaultMiddleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module.exports = function (
this.app.use(bodyParser.urlencoded({ extended: false }));
this.app.use(bodyParser.json());
this.app.use(methodOverride());
this.app.use(expressDevice.capture());
this.app.use(expressDevice.capture({ parseUserAgent: true }));
Copy link
Collaborator

@acolchado acolchado Apr 4, 2024

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?

Copy link
Contributor Author

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 return type: 'phone', name: '', with this change it will return type: 'phone', name: 'iPhone'. Might be good for logging/stats or other reasons.

}

}
Expand Down
28 changes: 11 additions & 17 deletions src/middleware/ErrorMiddleware.js
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);

Expand All @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assigning fallback statusCode of 0 is not necessary, it's only use to check it the status code exists in EXCLUDE_STATUSCODE_FROM_LOGS, which doesn't contains null or undefined.

According to the unit test coverage, the || 0 "branch" is never executed

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow, when using the middleware as expected, this.EXCLUDE_STATUSCODE_FROM_LOGS will always be [404], and calling some on this array with an undefined value is the same as calling it with 0, i.e. it returns false.
image

When calling the method directly, e.g. ErrorMiddleware.logErrorState({statusCode: 404}), this.EXCLUDE_STATUSCODE_FROM_LOGS is undefined and will always return false, which obviously is not expected.

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 = {};
}
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The updated logic does the same thing:
image


err.data = _assignIn(err.data, {
url: req.url
});
err.data = _assignIn(
{ ...err.data },
{
url: req.url,
},
);
}

sendTextResponse(err, req, res) {
Expand Down
13 changes: 4 additions & 9 deletions src/middleware/PromiseMiddleware.js
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);

Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the "default" value of {} to a "fallback" value because the default value only gets used when the argument is undefined or not provided, but if null is provided, it will fail.

return Promise.props(properties ?? {})
.then((props) => this.render(view, props))
.catch(this.req.next);
};
Expand All @@ -42,7 +37,7 @@ module.exports = function (
},
json: () => {
this.json(results);
}
},
});
})
.catch(this.req.next);
Expand Down
18 changes: 6 additions & 12 deletions src/webserver/BaseWebServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified logic because port property is always defined for this.server.address()

return this.server?.address()?.port ?? config.Port;
}

registerDefaultMiddleware() {
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 5 additions & 1 deletion test/fixtures/controllers/MockController.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@ module.exports = function (BaseController) {
class MockController extends BaseController {
configure(app) {
super.configure(app);
app.get("/", (req, res) => { res.status(200).send("SomeIndex") });
app.get("/", this.handleIndexRoute);
app.get("/with-error", (req, res) => { throw new Error("Throwing a basic error") });
}

handleIndexRoute(req, res) {
res.status(200).send("SomeIndex");
}
}

return new MockController();
Expand Down
31 changes: 27 additions & 4 deletions test/fixtures/controllers/PromiseMiddlewareTestController.js
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`);

Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

   const ejsMock = {
  render: jest.fn()
};

    injector()
      .addDependency('ejs', ejsMock, true)
      .inject((TasksPrinter) => {
        this.TasksPrinter = TasksPrinter;
      });

Copy link
Contributor Author

@otrreeves otrreeves Apr 5, 2024

Choose a reason for hiding this comment

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

app.set('view engine', ...); is not needed if the view name includes the extension (which I've done to remove it), but setting the views directory is still needed even when mocking the view engine, otherwise express throws Internal Server Error

From my experience trying to implement your injection suggestion, express doesn't seem pick up an injected view engine, even if it's a known engine like ejs, you have to register the engine via app.engine(...)

app.get('/promise-middleware-test--jsonasync', this.getJsonAsync);
app.get('/promise-middleware-test--renderasync', this.getRenderAsync);
app.get('/promise-middleware-test--sendAsync', this.getSendAsync);
Expand All @@ -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) {
Expand All @@ -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'));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this is needed.

Copy link
Contributor Author

@otrreeves otrreeves Apr 4, 2024

Choose a reason for hiding this comment

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

express().response.formatAsync expects 2 arguments, this fixes a bug.

}

Expand Down
9 changes: 9 additions & 0 deletions test/fixtures/controllers/views/home.ott
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
9 changes: 9 additions & 0 deletions test/fixtures/controllers/views/microapp.ott
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
14 changes: 11 additions & 3 deletions test/unit/middleware/ErrorMiddleware.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ describe('ErrorMiddleware', function () {
let htmlErrorRenderRenderSpy, errorMiddlewareSendTextResponseSpy, errorMiddlewareSendHtmlResponseSpy, errorMiddlewareSendJsonResponseSpy, loggerErrorSpy;

beforeEach(() => {
injector().inject((ErrorMiddleware, HTTPService,
TestWebServer, HtmlErrorRender, Logger, config) => {
injector().inject((ErrorMiddleware, HTTPService, TestWebServer, HtmlErrorRender, Logger, config) => {
this.ErrorMiddleware = ErrorMiddleware;
this.HTTPService = HTTPService;
this.TestWebServer = TestWebServer;
Expand All @@ -13,6 +12,8 @@ describe('ErrorMiddleware', function () {

this.mockPort = 9080;

Logger.useNoop();

htmlErrorRenderRenderSpy = jest.spyOn(this.HtmlErrorRender, 'render');
errorMiddlewareSendTextResponseSpy = jest.spyOn(this.ErrorMiddleware, 'sendTextResponse');
errorMiddlewareSendHtmlResponseSpy = jest.spyOn(this.ErrorMiddleware, 'sendHtmlResponse');
Expand Down Expand Up @@ -46,7 +47,7 @@ describe('ErrorMiddleware', function () {
expect.any(String),
expect.any(String),
expect.any(String),
{ url: expectUrl }
{ url: expectUrl },
);
};
});
Expand Down Expand Up @@ -164,4 +165,11 @@ describe('ErrorMiddleware', function () {
});
});
});

it("should not throw an error when logErrorStack is called without error argument and log 'empty' error", () => {
this.ErrorMiddleware.logErrorStack();

expect(loggerErrorSpy).toHaveBeenCalledTimes(1);
expect(loggerErrorSpy).toHaveBeenCalledWith({}, '\n', '', '\n', '');
});
});
18 changes: 17 additions & 1 deletion test/unit/middleware/NoCacheMiddleware.spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
describe('NoCacheMiddleware', function () {

beforeEach(() => {
return injector().inject((NoCacheMiddleware) => {
this.NoCacheMiddleware = NoCacheMiddleware;
Expand All @@ -9,4 +8,21 @@ describe('NoCacheMiddleware', function () {
it('should exist', () => {
expect(this.NoCacheMiddleware).toBeDefined();
});

it('should set no-cache headers', () => {
const response = {
headers: { Expires: 5184000 },
header: (name, value) => {
response.headers[name] = value;
},
};

this.NoCacheMiddleware({}, response, () => {});

expect(response.headers).toStrictEqual({
'Cache-Control': 'no-cache, no-store, must-revalidate',
Pragma: 'no-cache',
Expires: 0,
});
});
});
Loading
Loading