Update tests to match the testing guidelines #62
Conversation
gasperzgonec
requested review from
radovanjorgic,
patricijabrecko and
a team
as code owners
gasperzgonec
force-pushed
the
gasperz/ISS-202839
branch
from
0fff979 to
705c63e
Compare
gasperzgonec
force-pushed
the
gasperz/ISS-202839
branch
2 times, most recently
from
b28272a to
b6838fe
Compare
| @@ -75,7 +75,6 @@ describe('WorkerAdapter', () => { | |||
|
|
|||
| describe('streamAttachments', () => { | |||
| it('should process all artifact batches successfully', async () => { | |||
| // Arrange | |||
There was a problem hiding this comment.
This is about line 76:
We can use WorkerAdapter.streamAttachments.name
…iple test files. Updated test names to include '[edge]' prefix for clarity and consistency.
gasperzgonec
force-pushed
the
gasperz/ISS-202839
branch
from
7884599 to
81b310c
Compare
| testPathIgnorePatterns: [ | ||
| './dist/', | ||
| // Exclude timeout tests by default - they should only run with test:full or test:cov | ||
| './src/tests/timeout-handling/' |
There was a problem hiding this comment.
I have just checked and tests last ~20s longer when we don't ignore timeout-handling. I suggest not ignoring them, we have them for purpose and this can easily tell us if we broke something. Also these are only "e2e" tests we currently have.
| "test": "jest --silent --verbose=false", | ||
| "test:full": "jest --forceExit --runInBand --testPathIgnorePatterns='./dist/' --silent --verbose=false", | ||
| "test:cov": "jest --forceExit --coverage --runInBand --testPathIgnorePatterns='./dist/' --silent --verbose=false" |
There was a problem hiding this comment.
I think you are "over-engineering" this a bit. For simplicity, can we keep test as main script that will be mostly used (in CI and by developers) and remove others if not useful/needed. TBH I am used to often run npm run build and npm run test checks locally while developing.
| @@ -35,7 +35,7 @@ jobs: | |||
| run: npm ci | |||
|
|
|||
| - name: Run tests and get coverage | |||
| run: npm run test | |||
| run: npm run test:cov | |||
There was a problem hiding this comment.
If you remove this script please fix this too.
| @@ -11,7 +11,9 @@ | |||
| "start": "ts-node src/index.ts", | |||
| "lint": "eslint .", | |||
| "lint:fix": "eslint . --fix", | |||
| "test": "jest --forceExit --coverage --runInBand" | |||
| "test": "jest --silent --verbose=false", | |||
There was a problem hiding this comment.
Also, --silent flag can be a bit tricky, since it hides logs for all tests (even the failed ones) and if something breaks on CI it is hard to figure out what without logs. We need to fix existing tests to not log a lot of noise (we can't do anything for logs coming from SDK - logs like emitting events and similar).
| @@ -11,7 +11,9 @@ | |||
| "start": "ts-node src/index.ts", | |||
| "lint": "eslint .", | |||
| "lint:fix": "eslint . --fix", | |||
| "test": "jest --forceExit --coverage --runInBand" | |||
| "test": "jest --silent --verbose=false", | |||
There was a problem hiding this comment.
Isn't --verbose=false by default?
| @@ -66,7 +66,7 @@ describe('AttachmentsStreamingPool', () => { | |||
| jest.restoreAllMocks(); | |||
| }); | |||
|
|
|||
| describe('constructor', () => { | |||
| describe(AttachmentsStreamingPool.prototype.constructor.name, () => { | |||
There was a problem hiding this comment.
Maybe it won't be a bad idea to add how we should name these describe blocks in tests. Sometimes you can just use functionName.name or ClassName.method.name, but in some cases you need .prototype.name. Can we at least write down that these values ideally (if possible and makes sense) shouldn't be hardcoded?
Summary
Updated tests to match the testing guidelines specified inside of CONTRIBUTING.md file.
I have changed how the testing works.
Before, we had
npm run test, which ran all tests, including timeout tests, which took quite a while. I've removed them from the default testing and removed the coverage as well.Full and coverage tests include timeout tests and can be accessed using
test:fullandtest:cov. The actions should usetest:covby default and work as before.This speeds up testing a lot (3.5s vs 26s).
Connected Issues