Refactor of ExpressJS files #174

tuliomir · 2022-04-01T22:50:16Z

This PR aims to refactor this Express application into a more conventional format of separating files into routes, controllers and services.

Acceptance Criteria

The application should be structured in routes, controllers and services
Solves issue Refactor the express application #172
Solves issue Use eslint to lint the code #41

Risks and compromises

At the current state of the Integration Tests, the test results are sometimes failing due to timeouts and other minor failures related to excessive CPU consumption.

There are also some issues with the code coverage, raised by lgtm-com, that existed before this refactor and are outside its scope to change.

On commit 91be24f changes were made to the configurations to make most possible test pass. All tests not described below were passing at the time of that commit, which indicates the refactor was successful in not changing the application logic.

These changes were reverted on f628571 to ensure this PR is within its scope.

Failing Tests

Multisig - send tx (HTR) › Should send a transaction with minimum signatures
Multisig - send tx (HTR) › Should send a transaction with max signatures

Risks of merging with these failures

Both failures are still being investigated. But since the multisig feature is being blocked on the constants file, these failures pose no immediate risk to the users should this code reach production. Their errors have been pinpointed on the test results for 91be24f to be these:

{"success":false,"error":"OP_CHECKSIG: failed\nOP_CHECKSIG: failed\nOP_CHECKSIG: failed\nOP_CHECKSIG: failed\nStack left with more than one value"}
{"success":false,"error":"OP_DUP: empty stack"}

Security Checklist

Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.
Understand the risks of the failing tests and the need to make a separate task to fix them in the near future.

lgtm-com · 2022-04-01T22:58:02Z

This pull request introduces 1 alert and fixes 1 when merging b589964 into 1e6c692 - view on LGTM.com

new alerts:

1 for Use of externally-controlled format string

fixed alerts:

1 for Use of externally-controlled format string

codecov · 2022-04-01T23:01:30Z

Codecov Report

Merging #174 (91be24f) into dev (b995def) will increase coverage by 1.91%.
The diff coverage is 87.16%.

@@            Coverage Diff             @@
##              dev     #174      +/-   ##
==========================================
+ Coverage   83.93%   85.84%   +1.91%     
==========================================
  Files           7       19      +12     
  Lines         473      657     +184     
  Branches      104      132      +28     
==========================================
+ Hits          397      564     +167     
- Misses         69       84      +15     
- Partials        7        9       +2

Impacted Files	Coverage Δ
src/api-docs.js	`100.00% <ø> (ø)`
src/api-key-auth.js	`0.00% <ø> (ø)`
src/constants.js	`0.00% <0.00%> (ø)`
src/utils/generate_words.js	`0.00% <0.00%> (ø)`
src/index.js	`66.66% <55.55%> (-18.78%)`	⬇️
src/controllers/index.controller.js	`72.50% <72.50%> (ø)`
src/helpers/validations.helper.js	`83.33% <83.33%> (ø)`
src/routes/index.routes.js	`84.61% <84.61%> (ø)`
src/controllers/wallet.controller.js	`88.02% <88.02%> (ø)`
src/routes/wallet.routes.js	`90.32% <90.32%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b360e2...91be24f. Read the comment docs.

luislhl · 2022-04-05T20:03:37Z

package.json

@@ -24,6 +24,7 @@
    "test_network_down": "docker-compose -f ./__tests__/integration/docker-compose.yml down",
    "dev": "nodemon --exec babel-node index.js",
    "start": "babel-node src/index.js",
+    "lint": "eslint . --fix",


Shouldn't we run this in Github Actions? (without the --fix option, maybe)

Good idea! Done on 318dad2

This reverts commit 7d3ccd7.

codecov · 2022-04-06T14:34:04Z

Codecov Report

Merging #174 (f785b7a) into dev (b995def) will increase coverage by 1.84%.
The diff coverage is 87.11%.

@@            Coverage Diff             @@
##              dev     #174      +/-   ##
==========================================
+ Coverage   83.93%   85.78%   +1.84%     
==========================================
  Files           7       19      +12     
  Lines         473      647     +174     
  Branches      104      132      +28     
==========================================
+ Hits          397      555     +158     
- Misses         69       83      +14     
- Partials        7        9       +2

Impacted Files	Coverage Δ
src/api-docs.js	`100.00% <ø> (ø)`
src/api-key-auth.js	`0.00% <ø> (ø)`
src/constants.js	`0.00% <0.00%> (ø)`
src/utils/generate_words.js	`0.00% <0.00%> (ø)`
src/index.js	`66.66% <55.55%> (-18.78%)`	⬇️
src/controllers/index.controller.js	`72.50% <72.50%> (ø)`
src/routes/index.routes.js	`84.61% <84.61%> (ø)`
src/controllers/wallet/wallet.controller.js	`87.63% <87.63%> (ø)`
src/routes/wallet/wallet.routes.js	`90.32% <90.32%> (ø)`
src/helpers/tx.helper.js	`92.00% <92.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b360e2...f785b7a. Read the comment docs.

__tests__/integration/multisig.test.js

__tests__/test-utils.js

setupTests-integration.js

setupTests.js

pedroferreira1 · 2022-04-07T01:02:05Z

src/routes/wallet.routes.js

+ */
+walletRouter.get(
+  '/address-index',
+  query('address').isString(),


What method is being executed here? If we still don't have tests for this route, we should add it (not in this PR, of course).

This missing method is what was causing errors on the integration tests for send-tx. I agree we should add integration tests dedicated to these basic address routes in the near future to improve our coverage.

This was fixed on d829feb

Can you create an issue for these tests that don't exist?

Created issue #176 - Create tests for basic query routes

src/helpers/validations.helper.js

src/controllers/index.controller.js

r4mmer

I like the organization but some files were src/<section>/<name>.<section>.js and it felt repetitive.

Do you think we should remove the second <section>? Or is there any reason to keep it?

r4mmer · 2022-04-07T16:31:20Z

.github/workflows/lint.yml

+      # This file is required by most controllers and triggers "import/no-unresolved" if missing
+      - name: Copy config file
+        run: cp ./__tests__/integration/configuration/config-fixture.js ./src/config.js
+
+      - name: Lint
+        run: npm run lint


Why not just add these steps before testing on the main workflow?

Since our tests are currently not green, I tried to create the minimum possible interference with them. So I created a new workflow yml.

But now that you mention, I see our wallet mobile follows this practice of running the lint within the main workflow.

I agree that's a good idea, but I would rather make all tests green before making this move.
What do you think?

I think the interference caused by linters is not that big. We will be able to differentiate failures caused by them or by the tests.

I tried adding the linter to the main workflow on a555448, but it broke when tried to run on Node 8.x, possibly also on 10.x

We could use additional actions or configurations to enable the linting only on specific node versions, but I think there is little benefit for this extra complexity.

Do you have any suggestions on this?

r4mmer · 2022-04-07T16:33:15Z

.github/workflows/integration-test.yml

@@ -6,10 +6,10 @@ on:
      - dev
    tags:
      - v*
-    pull_request:


Unrelated to line

What's your opinion on running the integration tests after unit tests (on the main workflow)?
If the unit tests fail we don't need to run integration.

I like the idea of only running the integration tests after the unit ones, but maybe not on the same workflow.

Both tests have a timeout of 40 minutes and generate great amounts of logs. While we're trying to fine tune them, I think it's more helpful to keep them on isolated runs with separate outputs. The cost benefit may not be a relevant trade-off for the isolation of problems while in active development.

I think that with some optimizations on the headless, the tests and the underlying lib we could join those tests in the future. But while we don't get to that point, maybe other solutions like the wait on check action or the workflow_run property could be good alternatives to this ordering of actions.

What do you think?

r4mmer · 2022-04-07T17:09:33Z

setupTests-integration.js

+/* eslint-disable global-require */
 import * as Path from 'path';


Maybe import only parse since it's the only one used.

Good point. Done on e0b7301

I guess the idea with this import change is that you would be able to remove the comment above that disables the lint check. /* eslint-disable global-require */

I think you should remove it now that you've changed the import.

Actually this global-require is there because of lines 22 and 33, and it's still required.

This issue is being discussed on a thread below about the other setup file, but the reasons are the same.

r4mmer · 2022-04-07T17:11:49Z

setupTests.js

+/* eslint-disable import/no-extraneous-dependencies,global-require */
+import TestUtils from './__tests__/test-utils';


Why not remove this?

These are lint rules that I found it better to ignore for now, since fixing them could create confusion or take too long to investigate the proper way to implement them:

import/no-extraneous-dependencies

This accuses the mock-socket dependency to be not explicitly declared on the package.json dependencies. Looking at the rules we follow from airbnb-base, I see that the setup filename we've chosen is not within their convention.

At a future time we could move the setup files into the __tests__ folder and this would be solved.

global-require

On lines 18 and 29 we require javascript files within a very controlled clojure scope on Jest. If we imported these files into global variables and tried to use them in the mock factory method, we would get the following error:

babel-plugin-jest-hoist: The module factory of `jest.mock()` is not allowed to reference any out-of-scope variables.

r4mmer · 2022-04-07T17:16:59Z

src/services/wallets.service.js

+ * LICENSE file in the root directory of this source tree.
+ */
+
+// TODO: Make this a map instead of a common object. It's safer.


TODO on this PR or for later?

To do later. This is a comment to reinforce this Use of externally-controlled format string issue raised by LGTM bot.

r4mmer · 2022-04-07T17:24:47Z

src/routes/wallet.routes.js

+ */
+walletRouter.post('/stop', stop);
+
+walletRouter.use('/tx-proposal', txProposalRouter);


Maybe put this after the middleware on the start of the file.

Done on e0b7301

tuliomir · 2022-04-07T20:41:12Z

@r4mmer about both the file name and the folder containing the <section>, this is a convention I see on many articles and projects on ExpressJS.

I believe the idea is to help when the developer is working on multiple levels of the same part of the project. For example, instead of having three wallet.js files open, one would have the wallet.routes.js, wallet.controller.js and wallet.service.js, which removes ambiguity at a glance of the screen.

One example of this file structure is found on this article about ExpressJS project structures from 2020, and one example of a project boilerplate is on this repository of good practices last updated on 2021.

This reverts commit a555448. The linter is not working on Node 8.x

tuliomir added 4 commits April 1, 2022 15:14

refactor: created new files for wallet routes

3d709b6

refactor: new file structure and lint. Tests not passing

44b97c4

fix: mocked tests passing

33c71ff

fix: integration tests passing

4c1f231

tuliomir added the refactor label Apr 1, 2022

tuliomir self-assigned this Apr 1, 2022

fix: github workflow running integration tests

b589964

tuliomir changed the base branch from master to dev April 1, 2022 22:51

tuliomir mentioned this pull request Apr 4, 2022

Use eslint to lint the code #41

Closed

luislhl reviewed Apr 5, 2022

View reviewed changes

tuliomir added 2 commits April 5, 2022 19:10

perf: configuration improvements for test stability

7d3ccd7

style: linting

9a10bb1

tuliomir force-pushed the refactor/express-files branch from 8df9845 to 9a10bb1 Compare April 5, 2022 22:38

tuliomir added 3 commits April 5, 2022 20:08

ci: add lint to ci workflow

318dad2

ci: fix lint failure with config file

91be24f

Revert "perf: configuration improvements for test stability"

f628571

This reverts commit 7d3ccd7.

tuliomir added 2 commits April 6, 2022 16:32

revert: integration test configurations

e8ec339

refactor: joined some object destructuring

6f58215

HathorNetwork deleted a comment from lgtm-com bot Apr 6, 2022

tuliomir requested a review from r4mmer April 6, 2022 20:58

pedroferreira1 requested changes Apr 7, 2022

View reviewed changes

fix: missing controller on refactor

d829feb

HathorNetwork deleted a comment from lgtm-com bot Apr 7, 2022

tuliomir added 4 commits April 7, 2022 12:01

docs: adds explanation to commented variable

7ea530c

fix: reverts debugging code

35d3c67

refactor: removes unused function

aec0c8f

docs: adds Hathor Labs header to application files

2de3762

HathorNetwork deleted a comment from lgtm-com bot Apr 7, 2022

r4mmer requested changes Apr 7, 2022

View reviewed changes

pedroferreira1 mentioned this pull request Apr 7, 2022

feat: allow setting tx mining service apikey #175

Merged

1 task

refactor: fine tuning imports

e0b7301

pedroferreira1 self-requested a review April 7, 2022 19:07

pedroferreira1 approved these changes Apr 7, 2022

View reviewed changes

tuliomir requested a review from r4mmer April 7, 2022 19:27

tuliomir added 3 commits April 7, 2022 17:51

ci: moved lint to main workflow

a555448

Revert "ci: moved lint to main workflow"

1335f79

This reverts commit a555448. The linter is not working on Node 8.x

refactor: improved file structure on the wallet routes

f785b7a

HathorNetwork deleted a comment from lgtm-com bot Apr 8, 2022

r4mmer approved these changes Apr 8, 2022

View reviewed changes

tuliomir mentioned this pull request Apr 8, 2022

fix: lint and lgtm alerts #177

Open

9 tasks

tuliomir merged commit 4fc71df into dev Apr 8, 2022

tuliomir deleted the refactor/express-files branch April 8, 2022 18:32

This was referenced Apr 19, 2022

bump: v0.12.0 #180

Merged

Release: v0.12.0 #182

Merged

GraoMelo mentioned this pull request Apr 15, 2024

[Snyk] Security upgrade @hathor/wallet-lib from 0.9.0 to 0.20.0 GraoMelo/hathor-wallet-headless#15

Open

		/* eslint-disable global-require */
		import * as Path from 'path';

		/* eslint-disable import/no-extraneous-dependencies,global-require */
		import TestUtils from './__tests__/test-utils';

Refactor of ExpressJS files #174

Refactor of ExpressJS files #174

Uh oh!

Conversation

tuliomir commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Acceptance Criteria

Risks and compromises

Failing Tests

Risks of merging with these failures

Security Checklist

Uh oh!

lgtm-com bot commented Apr 1, 2022

Uh oh!

codecov bot commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

luislhl Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

r4mmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuliomir commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

tuliomir commented Apr 1, 2022 •

edited

Loading

codecov bot commented Apr 1, 2022 •

edited

Loading

luislhl Apr 5, 2022 •

edited

Loading

codecov bot commented Apr 6, 2022 •

edited

Loading

tuliomir commented Apr 7, 2022 •

edited

Loading