Skip to content
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

feat: tests (unit, e2e) #7

Merged
merged 15 commits into from
Jul 18, 2024
Merged

feat: tests (unit, e2e) #7

merged 15 commits into from
Jul 18, 2024

Conversation

typeWolffo
Copy link
Member

No description provided.

@typeWolffo typeWolffo requested review from mikoscz and k1eu July 15, 2024 20:19
@typeWolffo typeWolffo self-assigned this Jul 15, 2024
@typeWolffo typeWolffo force-pushed the jw/tests branch 2 times, most recently from befc4c9 to 8e58e1e Compare July 15, 2024 21:11
Comment on lines 100 to 111
let accessToken = "";
let refreshToken = "";

if (isArray(cookies)) {
cookies.forEach((cookie) => {
if (cookie.startsWith("access_token=")) {
accessToken = cookie;
} else if (cookie.startsWith("refresh_token=")) {
refreshToken = cookie;
}
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.npmjs.com/package/cookie
maybe something like this? seems usefull

Comment on lines 43 to 49
const response = await request(app.getHttpServer())
.get("/users")
.set("Cookie", cookies)
.expect(200);

expect(response.body.data).toBeDefined();
expect(Array.isArray(response.body.data)).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd rather test if the array contains correct data rather than toBeTrue

Comment on lines 60 to 61
expect(response.body.data).toBeDefined();
expect(response.body.data.id).toBe(testUser.id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, let's expect the correct object

Comment on lines 17 to 18
container = await new GenericContainer("postgres:14")
.withExposedPorts(5432)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe postgres:16 to match the dev env postgres

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also won't the 5432 port collide with the dev postgres port? or is it internal port?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as @k1eu we could use any available port(I think test containers do that by default) and then pass the url to config service.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://node.testcontainers.org/features/containers/#exposing-container-ports

as docs said: Testcontainers will automatically bind an available, random port on the host to each exposed container port. This is to avoid port conflicts when running tests quickly or in parallel.
so I think it's ok


const now = new Date().toISOString();

return {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should return only the user, credentials should be another factory, and we can add trait to the user factory eg. withCredentials() or sth like that.

return Factory.define<{ users: Users; credentials: Credentials }>(
({ params }) => {
const userId = faker.string.uuid();
const email = params.users?.email || faker.internet.email();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are not doing anything with params you don't need to do that all data put in params would override the factory generated data


export function createUsersFactory() {
return Factory.define<{ users: Users; credentials: Credentials }>(
({ params }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add onCreate which will save the user in the database

imports: [AppModule],
providers: [...customProviders],
})
.overrideProvider("DB")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to override db since we are injecting the url form the config service, we could just set the testcontainer url to the config and the rest should be fine.

getService: <T>(service: Type<T>) => T;
}

export async function createUnitTest(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick but the setup is more for integration/e2e since unit tests don't need app

})
.overrideProvider("DB")
.useValue(db)
.overrideProvider(JwtService)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave the normal implementation of the jwt service, create a helper that allow us to sing in as a given user, this way we can create e2e tests with multiple users in the workflow.

imports: [AppModule],
providers: [...customProviders],
})
.overrideProvider("DB")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

override config value

return jwtService.sign({ sub: userId });
}

export async function truncateAllDatabase(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: truncateAllTables is more precise but I am fine with this one

get: jest.fn((key: string) => {
switch (key) {
case "DATABASE_URL":
return process.env.DATABASE_URL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could just user config overrides for this one, you know some ppl export production db credentials sometimes :D

});

afterAll(async () => {
if (testContext.container) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it to the testContext as teardown or afterAll so we don't need to care in every test


it("should throw ConflictException if user already exists", async () => {
const email = "existing@example.com";
await db.insert(users).values({ email });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already commented in the factory file, but when you add onCreate then you could use factory to insert the user.

Comment on lines 75 to 85
const password = "password123";
const hashedPassword = await bcrypt.hash(password, 10);
const { users: userData, credentials: userCredentials } =
userFactory.build();

const [user] = await db.insert(users).values(userData).returning();
await db.insert(credentials).values({
...userCredentials,
userId: user.id,
password: hashedPassword,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move that to factory trait

Comment on lines 27 to 38
const { users: newUser, credentials } = userFactory.build();
testUser = await authService.register(newUser.email, credentials.password);
testCredentials = credentials;

const loginResponse = await request(app.getHttpServer())
.post("/auth/login")
.send({
email: newUser.email,
password: credentials.password,
});

cookies = loginResponse.headers["set-cookie"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a helper that would give as valid set of cookies instead so we don't need to call login endpoint in every test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or you could also add method to testagent .signAs(user)

export async function createE2ETest(customProviders: Provider[] = []) {
const { db, connectionString } = await setupTestDatabase();

process.env.DATABASE_URL = connectionString;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikoscz what do you think about overriding database URL like that? :D

Comment on lines +27 to +28
.withCredentials({ password: "password123" })
.build();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice ^^

.select()
.from(users)
.where(eq(users.email, user.email));
expect(savedUser).toBeDefined();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move it to the expect part of this test

Comment on lines 76 to 83
expect(updatedUser).toBeDefined();
expect(updatedUser.email).toBe("new@example.com");

const [dbUser] = await db
.select()
.from(users)
.where(eq(users.id, testUser.id));
expect(dbUser.email).toBe("new@example.com");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same a bit of reorder so the expect are last

@typeWolffo typeWolffo merged commit 0cff62a into jw/user-auth Jul 18, 2024
@typeWolffo typeWolffo deleted the jw/tests branch July 18, 2024 12:00
typeWolffo added a commit that referenced this pull request Jul 18, 2024
* feat: configure jest for TypeScript and test database setup

* feat: expand e2e tests for auth.controller and add new migration file

* feat: update password change functionality and schema

* feat: initialize test context and services

* feat: create unit test setup function

* feat: initialize test context and services for e2e tests

* feat: create e2e test setup function

* refactor: improve user and credential factories, add userWithCredentialFactory

* feat: update dependencies and add JWT_REFRESH_SECRET

* refactor: refactor user factory and add hashPassword function

* fix: update password handling in user controller e2e tests

* feat: refactor userFactory and authService.register

* chore: update assertions in auth and users service tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants