chore(tests): [RO-26598] Improve test coverage #83

otrreeves · 2024-04-04T03:23:25Z

Ticket

https://opentable.atlassian.net/browse/RO-26598

Change Summary

This PR will improve test coverage. When I was working on cleaning out some unnecessary dependencies, test coverage fell below the coverage-threshold so this will improve the coverage before removing the dependencies. As a plus, this should make removing the dependencies more fool-proof.

	Sceenshot
Before
After

…ttier instead of disabling formatOnSave

otrreeves · 2024-04-04T03:27:05Z

package.json

@@ -65,6 +65,7 @@
  "devDependencies": {
    "@types/jest": "29.5.12",
    "colors": "1.4.0",
+    "ejs": "3.1.9",


This is necessary for testing the express server's render method (called via renderAsync from PromiseMiddleware), which wasn't being tested at all previously. I also couldn't find usage within the OpenTable codebase, but this is a public repo so it could be used somewhere in the world.

The alternative would be to instruct the test coverage to ignore the code, but the code did contain errors...

It'd be better if you replace it with a mock and add it as an injection override. This is one of the more annoying dependencies in the system. :)

otrreeves · 2024-04-04T03:34:52Z

src/middleware/ErrorMiddleware.js

      });

      next();
    }

    logErrorStack(err) {
-      const statusCode = err.statusCode || 0;


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

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

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.

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

otrreeves · 2024-04-04T03:38:32Z

src/middleware/PromiseMiddleware.js

-      this.app.response.renderAsync = function (view, properties = {}) {
-        return Promise.props(properties)


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.

otrreeves · 2024-04-04T03:40:23Z

src/webserver/BaseWebServer.js

-      let port = config.Port;
-
-      if (this.server) {
-        port = this.server.address().port || port;
-      }
-
-      return port;


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

…r agent so it returns a value for the name field

acolchado

Overall it looks great. There are some problems introduced. The fact that code is not accessed because it's called locally only, doesn't mean something injecting it will not call it. So it's better to be extra safe. I think some of them were added as patches to fix the issues that this would reintroduce.

acolchado · 2024-04-04T07:11:46Z

src/middleware/DefaultMiddleware.js

@@ -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 }));


What is this for? And why was it working beore? what expectation does it change?

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.

acolchado · 2024-04-04T07:27:31Z

src/middleware/ErrorMiddleware.js

      });

      next();
    }

    logErrorStack(err) {
-      const statusCode = err.statusCode || 0;


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

acolchado · 2024-04-04T07:42:37Z

src/middleware/ErrorMiddleware.js

      }
    }

    appendRequestData(err, req) {
-      if (err.data == null) {
-        err.data = {};
-      }


I think this was a patch to something. The fact that it wasn't defaulted to an empty object.

The updated logic does the same thing:

acolchado · 2024-04-04T08:17:57Z

test/fixtures/controllers/PromiseMiddlewareTestController.js

    configure(app) {
      super.configure(app);
+
+      app.set('view engine', 'ejs');
+      app.set('views', `${__dirname}/views`);


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; });

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(...)

acolchado · 2024-04-04T08:22:10Z

test/fixtures/controllers/PromiseMiddlewareTestController.js

@@ -28,7 +38,7 @@ module.exports = function (BaseController, Promise) {
    }

    getFormatAsync(req, res) {
-      res.formatAsync(Promise.resolve('formatAsync success'));
+      res.formatAsync('innerHTML', Promise.resolve('formatAsync success'));


Not sure why this is needed.

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

acolchado · 2024-04-04T08:22:28Z

test/fixtures/controllers/views/home.ejs

+<body>
+  No render view-props provided
+</body>
+</html>


If you mock ejs in the injector, you don't need any of this.

Turns out mocking a view engine still requires view-files to exist, otherwise express throws Internal Server Error

… mock

otrreeves · 2024-04-05T03:11:47Z

test/fixtures/controllers/views/home.ott

Just a dummy view to test the view-engine, it's required event with a mocked view engine

otrreeves · 2024-04-05T03:11:51Z

test/fixtures/controllers/views/microapp.ott

Just a dummy view to test the view-engine, it's required event with a mocked view engine

otrreeves added 2 commits April 4, 2024 13:42

chore(tests): [RO-26598] Imporoved test coverage

ebe0638

chore(tests): [RO-26598] Updated .vscode/settings.json to disable Pre…

4d1c150

…ttier instead of disabling formatOnSave

otrreeves commented Apr 4, 2024

View reviewed changes

otrreeves requested a review from acolchado April 4, 2024 03:43

otrreeves added 2 commits April 4, 2024 16:50

chore(tests): [RO-26598] Added test for express-device

b4011f4

chore(tests): [RO-26598] Added option for express-device to parse use…

b916e19

…r agent so it returns a value for the name field

acolchado changed the title ~~Chore/ro 26598 improve test coverage~~ chore(): [RO-26598} Improve test coverage Apr 4, 2024

acolchado requested changes Apr 4, 2024

View reviewed changes

otrreeves added 3 commits April 5, 2024 13:15

chore(tests): [RO-26598] Addressed PR Feedback

99e21c3

chore(tests): [RO-26598] Updated coverage threshold

61c193b

chore(tests): [RO-26598] Update to make it clear the view engine is a…

61c62d7

… mock

otrreeves requested a review from acolchado April 5, 2024 03:10

otrreeves commented Apr 5, 2024

View reviewed changes

otrreeves changed the title ~~chore(): [RO-26598} Improve test coverage~~ chore(tests): [RO-26598] Improve test coverage Apr 5, 2024

acolchado approved these changes Apr 8, 2024

View reviewed changes

acolchado merged commit 939d3fc into main Apr 8, 2024
3 checks passed

acolchado deleted the chore/ro-26598-improve-test-coverage branch April 8, 2024 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(tests): [RO-26598] Improve test coverage #83

chore(tests): [RO-26598] Improve test coverage #83

otrreeves commented Apr 4, 2024 •

edited

Loading

otrreeves Apr 4, 2024 •

edited

Loading

acolchado Apr 4, 2024

otrreeves Apr 5, 2024

otrreeves Apr 4, 2024

acolchado Apr 4, 2024

otrreeves Apr 5, 2024

otrreeves Apr 4, 2024

otrreeves Apr 4, 2024

acolchado left a comment

acolchado Apr 4, 2024 •

edited

Loading

otrreeves Apr 4, 2024

acolchado Apr 4, 2024

acolchado Apr 4, 2024

otrreeves Apr 4, 2024

acolchado Apr 4, 2024

otrreeves Apr 5, 2024 •

edited

Loading

acolchado Apr 4, 2024

otrreeves Apr 4, 2024 •

edited

Loading

acolchado Apr 4, 2024

otrreeves Apr 5, 2024 •

edited

Loading

otrreeves Apr 5, 2024

otrreeves Apr 5, 2024

		this.app.response.renderAsync = function (view, properties = {}) {
		return Promise.props(properties)

chore(tests): [RO-26598] Improve test coverage #83

chore(tests): [RO-26598] Improve test coverage #83

Conversation

otrreeves commented Apr 4, 2024 • edited Loading

Ticket

Change Summary

otrreeves Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolchado left a comment

Choose a reason for hiding this comment

acolchado Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otrreeves Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otrreeves Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otrreeves Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otrreeves commented Apr 4, 2024 •

edited

Loading

otrreeves Apr 4, 2024 •

edited

Loading

acolchado Apr 4, 2024 •

edited

Loading

otrreeves Apr 5, 2024 •

edited

Loading

otrreeves Apr 4, 2024 •

edited

Loading

otrreeves Apr 5, 2024 •

edited

Loading