-
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 18 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 |
---|---|---|
@@ -0,0 +1,29 @@ | ||
#!/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=src/coverage --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 | ||
|
||
path="src/$target" | ||
command="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 $path --coverage --colors \ | ||
--reporters=jest-junit --reporters=default --runInBand \ | ||
--silent --colors --logHeapUsage --coverageDirectory=../coverage/$target --collectCoverageFrom=$path/**/*.js --forceExit" | ||
|
||
$command | ||
done | ||
} | ||
|
||
main | ||
thewatermethod marked this conversation as resolved.
Show resolved
Hide resolved
|
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", | ||
|
@@ -130,18 +130,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 | ||
} | ||
} | ||
}, | ||
|
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 |
||
})); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -575,7 +575,7 @@ describe('goal filtersToScopes', () => { | |
}); | ||
|
||
expect(found.length).toBe(6); | ||
expect(found[0].name).toContain('Goal 1'); | ||
expect(found.map((f) => f.name)).toContain('Goal 1'); | ||
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. again, this test is flaky so I'm trying to fix that |
||
}); | ||
|
||
it('filters out by region', async () => { | ||
|
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