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

[CRT-5] Classes API - E2E tests #59

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pierluca
Copy link
Contributor

@pierluca pierluca commented Nov 5, 2024

No description provided.

Comment on lines +6 to +21
const checkClassesInList = (expectedClasses, returnedClasses): void => {
expect(returnedClasses).toHaveLength(expectedClasses.length);
returnedClasses.forEach((returnedClass) => {
const { teacherId, ...referenceClass } = expectedClasses.find(
(c) => c.id == returnedClass.id,
);
expect(returnedClass).toMatchObject(referenceClass);

const {
type: _ignore_type,
email: _ignore_email,
...referenceTeacher
} = users.find((u) => u.id === teacherId)!;
expect(returnedClass.teacher).toMatchObject(referenceTeacher);
});
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can probably be done better, I would like to develop a library of matchers, but I believe it should be "good enough" for now :-)

id: defaultTeacher.id,
name: defaultTeacher.name,
},
sessions: [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the seed data doesn't have sessions for now, otherwise this would have to be computed too.

Base automatically changed from feature/CRT-5_api_class to main November 5, 2024 07:42
Copy link
Contributor

@Tyratox Tyratox left a comment

Choose a reason for hiding this comment

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

A few small comments :) Also I pushed a commit to exclude the tests from sonar's coverage reporting - this PR had a terrible coverage before because of the e2e-spec.ts file xD

Comment on lines +15 to +16
type: _ignore_type,
email: _ignore_email,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to assert the absence of those fields instead? (also fine for me if we add a comment and do it later)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they're there and I literally don't want them in the referenceTeacher :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But longer term, I'd like to have proper matchers for the various DTOs.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see ^^

Comment on lines +81 to +110
test("/classes (POST)", async () => {
const dto = { name: "New Class", teacherId: defaultTeacher.id };

const response = await request(app.getHttpServer())
.post("/classes")
.send(dto)
.expect(201);

expect(response.body).toEqual({ id: 1, ...dto });
});

test("/classes/:id (PATCH)", async () => {
const klass = classes[0];
const dto = { name: "Updated Class", teacherId: defaultTeacher.id };

const response = await request(app.getHttpServer())
.patch(`/classes/${klass.id}`)
.send(dto)
.expect(200);

expect(response.body).toEqual({ id: klass.id, ...dto });
});

test("/classes/:id (DELETE)", async () => {
const klass = classes[0];
const response = await request(app.getHttpServer())
.delete(`/classes/${klass.id}`)
.expect(200);

expect(response.body).toEqual(klass);
Copy link
Contributor

Choose a reason for hiding this comment

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

While at it, why not add a "not found" test for POST, PATCH & DELETE as well? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be helpful indeed ^^

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.

2 participants