-
Notifications
You must be signed in to change notification settings - Fork 44
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
Test Cases #151
base: feat/c4gt
Are you sure you want to change the base?
Test Cases #151
Conversation
Includes comments which are bit problematic need to discuss
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.
Where are the tests for query builder service?
impl/c-qube/src/main.ts
Outdated
@@ -1,7 +1,7 @@ | |||
import { NestFactory } from '@nestjs/core'; | |||
import { AppModule } from './app.module'; | |||
|
|||
async function bootstrap() { |
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.
Why have you exported this?
PK,,Index`; | ||
const mockReadFile = jest.fn().mockResolvedValue(mockFileContent); | ||
const csvFilePath = '/path/to/valid_file.csv'; | ||
await createDimensionGrammarFromCSVDefinition(csvFilePath, mockReadFile); |
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.
Make sure the function is returning the correct response as well
const csvFilePath = '/path/to/invalid_file.csv'; | ||
console.error = jest.fn(); | ||
const result = await createDimensionGrammarFromCSVDefinition(csvFilePath, mockReadFile); | ||
expect(result).toBeNull(); |
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.
Add a positive test case where the result is not null.
// const inputFilePath = 'test-input.csv'; | ||
// const outputFilePath = 'test-output.csv'; | ||
|
||
// // Create a sample input file |
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.
Uncomment and fix these test cases as well
it('should return "Hello World!"', () => { | ||
expect('Hello World!').toBe('Hello World!'); | ||
}); | ||
it('should return "Hello World!"', () => { |
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.
delete this expect('Hello World!').toBe('HelloWorld!') test
expect(result).toBe(4); | ||
}); | ||
|
||
it('should transform asynchronously', 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.
what exactly is the intention behind this test case?
const transformAsyncFn = service.stringToTransformAsync('(callback) => { throw new Error("Test Error"); }'); | ||
try { | ||
await transformAsyncFn(null, null, null); | ||
expect(true).toBe(false); |
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.
what is this???? 😱
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.
The purpose of this assertion is to ensure that the try...catch block is actually catching the error. If the try...catch block was not catching the error, then the expect statement would not fail and the test would pass even though the error was not actually caught. And this test case is covering the catch error line under stringToTransformAsync property .
try { | ||
await transformAsyncFn(null, null, null); | ||
expect(true).toBe(false); | ||
} catch (error) {} |
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.
where is the error expected?
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.
Added expect statement for this .
const callback = jest.fn(); | ||
try { | ||
transformSyncFn(callback, null, null); | ||
expect(true).toBe(false); |
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.
what is happening here?? 😱
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.
The purpose of this assertion is to ensure that the try...catch block is actually catching the error. If the try...catch block was not catching the error, then the expect statement would not fail and the test would pass even though the error was not actually caught. And this test case is covering the catch error line under stringToTransformSync property .
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 is not making sense to me kindly explain this during today's call.
impl/c-qube/src/utils/hash.spec.ts
Outdated
// eslint-disable-next-line @typescript-eslint/no-var-requires | ||
|
||
jest.mock('yargs', () => ({ |
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.
why are yargs tests inside hash.spec.ts? what is the relevance for adding them here?
This reverts commit 2ccb7f4.
@shashwatm1111 resolve all the comments and merge conflicts on this PR to get this into a mergable state. |
Added test case #79