-
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
[TTAHUB-1037] Identify and fix memory leak issue #1050
Changes from 35 commits
613c42f
3e8304b
11280e8
6a6cb70
0f7d1ae
309a505
7f7485b
8a08f35
096f3be
4b2a699
666429a
2207ed4
9b6f42a
4798df5
c62c001
dd8a9d9
0575b37
a509c25
d8997f8
25c5a78
6d4b123
750cf39
be1c0ca
beaee60
4a14844
6a1018f
fbff34d
d4de8a1
8c72a53
e19e995
ca2a9e5
ba77179
97c5fdf
056f758
5efdd67
149185c
e2a1ec2
2ee7033
48bb1ef
b83f226
dd6a4b3
9ab7a34
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,2 +1,2 @@ | ||
FROM node:16.17.0 | ||
WORKDIR /app | ||
WORKDIR /app |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,10 @@ You may run into some issues running the docker commands on Windows: | |
* If you run into `Permission Denied` errors see [this issue](https://github.com/docker/for-win/issues/3385#issuecomment-501931980) | ||
* You can try to speed up execution time on windows with solutions posted to [this issue](https://github.com/docker/for-win/issues/1936) | ||
|
||
### Coverage reports | ||
|
||
On the frontend, the lcov and HTML files are generated as normal, however on the backend, the folders are tested separately. The command `yarn coverage:backend` will concatenate the lcov files and also generate an HTML file. However, this provess requires `lcov` to be installed on a user's computer. On Apple, you can use Homebrew - `brew install lcov`. On a Windows machine, you will likely have to use WSL. | ||
|
||
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. Would like to expand this if a Windows person wants to tackle getting it working in that environment |
||
## Yarn Commands | ||
|
||
| Docker Command | Description| Host Command | Local only Command | | ||
|
@@ -118,6 +122,7 @@ You may run into some issues running the docker commands on Windows: | |
| | Run `yarn lint:ci` for both the frontend and backend | `yarn lint:all`| | | ||
| | Host the open api 3 spec using [redoc](https://github.com/Redocly/redoc) at `localhost:5003` | `yarn docs:serve` | | | ||
| | Run cucumber tests | `yarn cucumber` | | | ||
| | Collect backend coverage report | `yarn coverage:backend` || | ||
|
||
## Infrastructure | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
#!/bin/bash | ||
|
||
main(){ | ||
# start collecting our coverage files | ||
coverage_files=("coverage/src/root/lcov.info") | ||
|
||
# then list through the folders and run the tests | ||
targets=("lib" "middleware" "models" "policies" "routes" "scopes" "services" "tools" "widgets") | ||
|
||
for target in "${targets[@]}"; do | ||
coverage_files+=("coverage/src/$target/lcov.info") | ||
done | ||
|
||
# concatenate all the coverage files into one | ||
for f in "${coverage_files[@]}"; do | ||
cat "$f" >> lcov.info | ||
done | ||
|
||
# generate the html report | ||
if lcov -v; then | ||
genhtml lcov.info --output-directory coverage | ||
else | ||
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. obviously this may present a problem for windows users - there may be a more windows friendly way to generate HTML from the lcov. Perhaps just dragging the files into Access. Who knows? 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 if helpful, but leaving this here: |
||
echo "lcov not found. Please install lcov to generate coverage report" | ||
fi | ||
} | ||
|
||
main |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,19 +80,84 @@ main() { | |
fi | ||
|
||
if [[ "$opt" == "backend" || -z "$opt" ]]; then | ||
# Test backend | ||
command="yarn test:ci --colors" | ||
if [ $verbose == "false" ]; then | ||
command="yarn test:ci --silent --colors" | ||
fi | ||
echo | ||
log "Running backend tests" | ||
if [[ $docker == "true" ]]; then | ||
docker exec test-backend bash -c "$command" | ||
else | ||
$command | ||
fi | ||
check_exit "$?" | ||
# Test backend | ||
echo | ||
log "Running backend tests" | ||
|
||
# remove existing coverage folder, since we are changing how things are structured | ||
rm -f -rf coverage | ||
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. can omit the |
||
|
||
# first get the tests in the root directory | ||
node_modules/.bin/cross-env \ | ||
JEST_JUNIT_OUTPUT_DIR=reports \ | ||
JEST_JUNIT_OUTPUT_NAME=unit.xml \ | ||
POSTGRES_USERNAME=postgres \ | ||
POSTGRES_DB=ttasmarthub \ | ||
CURRENT_USER_ID=5 \ | ||
CI=true \ | ||
node \ | ||
--expose-gc \ | ||
./node_modules/.bin/jest \ | ||
src/*.js \ | ||
--coverage \ | ||
--colors \ | ||
--reporters=jest-junit \ | ||
--reporters=default \ | ||
--runInBand \ | ||
--silent \ | ||
--colors \ | ||
--logHeapUsage \ | ||
--coverageDirectory="$(pwd)"/coverage/src/root \ | ||
--collectCoverageFrom=src/*.js \ | ||
--forceExit | ||
|
||
if [[ $docker == "true" ]]; then | ||
docker exec test-backend bash -c "$command" | ||
else | ||
$command | ||
fi | ||
|
||
check_exit "$?" | ||
|
||
# then list through the folders and run the tests | ||
targets=("lib" "middleware" "models" "policies" "routes" "scopes" "services" "tools" "widgets") | ||
|
||
for target in "${targets[@]}"; do | ||
# jest command to | ||
# - run tests in the target folder | ||
# - collect coverage from the target folder | ||
# - output coverage relative to the target folder | ||
# - some other useful flags | ||
node_modules/.bin/cross-env \ | ||
JEST_JUNIT_OUTPUT_DIR=reports \ | ||
JEST_JUNIT_OUTPUT_NAME=unit.xml \ | ||
POSTGRES_USERNAME=postgres \ | ||
POSTGRES_DB=ttasmarthub \ | ||
CURRENT_USER_ID=5 \ | ||
CI=true \ | ||
node \ | ||
--expose-gc \ | ||
./node_modules/.bin/jest \ | ||
src/"$target" \ | ||
--coverage \ | ||
--colors \ | ||
--reporters=jest-junit \ | ||
--reporters=default \ | ||
--runInBand \ | ||
--silent \ | ||
--colors \ | ||
--logHeapUsage \ | ||
--coverageDirectory="$(pwd)"/coverage/src/"$target" \ | ||
--collectCoverageFrom=src/"$target"/**/*.js \ | ||
--forceExit | ||
|
||
if [[ $docker == "true" ]]; then | ||
docker exec test-backend bash -c "$command" | ||
else | ||
$command | ||
fi | ||
check_exit "$?" | ||
done | ||
fi | ||
|
||
if [[ "$opt" == "frontend" || -z "$opt" ]]; then | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
#!/bin/bash | ||
|
||
main(){ | ||
# first get the tests in the root directory | ||
node_modules/.bin/cross-env \ | ||
JEST_JUNIT_OUTPUT_DIR=reports \ | ||
JEST_JUNIT_OUTPUT_NAME=unit.xml \ | ||
POSTGRES_USERNAME=postgres \ | ||
POSTGRES_DB=ttasmarthub \ | ||
CURRENT_USER_ID=5 \ | ||
CI=true \ | ||
node \ | ||
--expose-gc \ | ||
./node_modules/.bin/jest \ | ||
src/*.js \ | ||
--coverage \ | ||
--colors \ | ||
--reporters=jest-junit \ | ||
--reporters=default \ | ||
--runInBand \ | ||
--silent \ | ||
--colors \ | ||
--logHeapUsage \ | ||
--coverageDirectory="$(pwd)"/coverage/src \ | ||
--collectCoverageFrom=src/*.js \ | ||
--forceExit | ||
|
||
# then list through the folders and run the tests | ||
targets=("lib" "middleware" "models" "policies" "routes" "scopes" "services" "tools" "widgets") | ||
|
||
for target in "${targets[@]}"; do | ||
# jest command to | ||
# - run tests in the target folder | ||
# - collect coverage from the target folder | ||
# - output coverage relative to the target folder | ||
# - some other useful flags | ||
|
||
node_modules/.bin/cross-env \ | ||
JEST_JUNIT_OUTPUT_DIR=reports \ | ||
JEST_JUNIT_OUTPUT_NAME=unit.xml \ | ||
POSTGRES_USERNAME=postgres \ | ||
POSTGRES_DB=ttasmarthub \ | ||
CURRENT_USER_ID=5 \ | ||
CI=true \ | ||
node \ | ||
--expose-gc \ | ||
./node_modules/.bin/jest \ | ||
src/"$target" \ | ||
--coverage \ | ||
--colors \ | ||
--reporters=jest-junit \ | ||
--reporters=default \ | ||
--runInBand \ | ||
--silent \ | ||
--colors \ | ||
--logHeapUsage \ | ||
--coverageDirectory="$(pwd)"/coverage/src/"$target" \ | ||
--collectCoverageFrom=src/"$target"/**/*.js \ | ||
--forceExit | ||
done | ||
} | ||
|
||
main |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ | |
"server:debug": "nodemon --inspect=0.0.0.0:9229 src/index.js --exec babel-node", | ||
"client": "yarn --cwd frontend start", | ||
"test": "jest src --runInBand", | ||
"test:ci": "cross-env JEST_JUNIT_OUTPUT_DIR=reports JEST_JUNIT_OUTPUT_NAME=unit.xml POSTGRES_USERNAME=postgres POSTGRES_DB=ttasmarthub CURRENT_USER_ID=5 CI=true jest src --coverage --reporters=default --reporters=jest-junit --runInBand", | ||
"test:ci": "cross-env JEST_JUNIT_OUTPUT_DIR=reports JEST_JUNIT_OUTPUT_NAME=unit.xml POSTGRES_USERNAME=postgres POSTGRES_DB=ttasmarthub CURRENT_USER_ID=5 CI=true node --expose-gc ./node_modules/.bin/jest src --coverage --reporters=jest-junit --reporters=default --runInBand", | ||
"test:all": "yarn test:ci && yarn --cwd frontend test:ci", | ||
"lint": "eslint src", | ||
"lint:ci": "eslint -f eslint-formatter-multiple src", | ||
|
@@ -81,7 +81,8 @@ | |
"changeReportStatus:local": "./node_modules/.bin/babel-node ./src/tools/changeReportStatusCLI.js", | ||
"makecolors": "node ./src/makecolors.js", | ||
"restoreTopics": "node ./build/server/tools/restoreTopicsCLI.js", | ||
"restoreTopics:local": "./node_modules/.bin/babel-node ./src/tools/restoreTopicsCLI.js" | ||
"restoreTopics:local": "./node_modules/.bin/babel-node ./src/tools/restoreTopicsCLI.js", | ||
"coverage:backend": "./bin/build-coverage-report" | ||
}, | ||
"repository": { | ||
"type": "git", | ||
|
@@ -101,7 +102,8 @@ | |
"node-fetch": "^2.6.7", | ||
"protobufjs": "^6.11.3", | ||
"got": "^11.8.5", | ||
"undici": "^5.8.0" | ||
"undici": "^5.8.0", | ||
"selenium-webdriver": "4.3.0" | ||
}, | ||
"eslintConfig": { | ||
"extends": [ | ||
|
@@ -130,18 +132,35 @@ | |
}, | ||
"jest": { | ||
"testPathIgnorePatterns": [ | ||
"<rootDir>/frontend/" | ||
"<rootDir>/frontend/", | ||
"<rootDir>node_modules/", | ||
"<rootDir>/build/", | ||
"<rootDir>/src/makecolors.js", | ||
"<rootDir>/src/newrelic.js", | ||
"<rootDir>/src/testUtils.js", | ||
"<rootDir>/src/logger.js", | ||
"<rootDir>/src/*/coverage/", | ||
"<rootDir>/src/coverage/" | ||
], | ||
"testTimeout": 30000, | ||
"coveragePathIgnorePatterns": [ | ||
"<rootDir>/src/middleware/newRelicMiddleware.js" | ||
"<rootDir>/src/middleware/newRelicMiddleware.js", | ||
"<rootDir>/frontend/", | ||
"<rootDir>node_modules", | ||
"<rootDir>/build/", | ||
"<rootDir>/src/makecolors.js", | ||
"<rootDir>/src/newrelic.js", | ||
"<rootDir>/src/testUtils.js", | ||
"<rootDir>/src/logger.js", | ||
"<rootDir>/src/*/coverage/", | ||
"<rootDir>/src/coverage/" | ||
], | ||
"coverageThreshold": { | ||
"global": { | ||
"statements": 85, | ||
"functions": 70, | ||
"branches": 70, | ||
"lines": 85 | ||
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 obviously not ideal, but I'm not sure this PR should involve bringing every folder up to a higher test threshold. 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 I get why these new thresholds were chosen (9 sep. folders, 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. That's giving me a great deal more credit than I probably deserve - I just lowered them until they'd pass. I just moved them back up to about the highest possible values right now. And yes, you are correct, each test/folder only needs to clear that threshold specified in this config. |
||
"statements": 10, | ||
"functions": 10, | ||
"branches": 10, | ||
"lines": 20 | ||
} | ||
} | ||
}, | ||
|
@@ -184,7 +203,7 @@ | |
"puppeteer": "^13.1.1", | ||
"puppeteer-select": "^1.0.3", | ||
"redoc-cli": "^0.13.2", | ||
"selenium-webdriver": "4.0.0", | ||
"selenium-webdriver": "4.3.0", | ||
"supertest": "^6.1.3" | ||
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. |
||
}, | ||
"dependencies": { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -189,14 +189,12 @@ describe('Audit System', () => { | |
throw (err); | ||
} | ||
|
||
expect(routines) | ||
.toEqual([ | ||
{ name: 'ZALFTests' }, | ||
{ name: 'ZALNoDeleteFTests' }, | ||
{ name: 'ZALNoTruncateFTests' }, | ||
{ name: 'ZALNoUpdateFTests' }, | ||
{ name: 'ZALTruncateFTests' }, | ||
]); | ||
const routineNames = routines.map((routine) => routine.name); | ||
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. trying to make this test less fragile |
||
expect(routineNames).toContain('ZALFTests'); | ||
expect(routineNames).toContain('ZALNoDeleteFTests'); | ||
expect(routineNames).toContain('ZALNoTruncateFTests'); | ||
expect(routineNames).toContain('ZALNoUpdateFTests'); | ||
expect(routineNames).toContain('ZALTruncateFTests'); | ||
|
||
const hooks = [ | ||
'beforeBulkCreate', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,9 @@ jest.mock('../../services/activityReports', () => ({ | |
jest.mock('../../services/objectives', () => ({ | ||
saveObjectivesForReport: jest.fn(), | ||
getObjectivesByReportId: jest.fn(), | ||
})); | ||
|
||
jest.mock('../../services/userSettings', () => ({ | ||
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 got gunked up when we merged |
||
userSettingOverridesById: jest.fn(), | ||
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 believe this is just fixing a bad merge |
||
})); | ||
|
||
|
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.
Do we have a way of accessing these artifacts?
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've been using a bash script to fetch all coverage files, but I think I'd prefer to just pass lcov.info to genhtml locally as we just talked about on slack.
fwiw this is that script
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 would be a cool thing to add to our scripts, especially if you had the time to write up a brief instruction for it