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

Fieldtrip api integration tests #10

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

Conversation

alaa-yahia
Copy link
Collaborator

No description provided.

Copy link
Owner

@djgrant djgrant 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 comments but otherwise lovely stuff :)

.circleci/config.yml Outdated Show resolved Hide resolved
apps/server/src/routers/api.ts Outdated Show resolved Hide resolved
apps/server/src/services/courses.ts Outdated Show resolved Hide resolved
apps/server/src/services/migrate.ts Outdated Show resolved Hide resolved
apps/server/tsconfig.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
apps/server/src/routers/api.ts Show resolved Hide resolved
apps/server/src/services/courses.ts Outdated Show resolved Hide resolved
apps/server/test/api.test.ts Show resolved Hide resolved
apps/server/test/api.test.ts Outdated Show resolved Hide resolved
course_id: "course",
})
).toBeNull;
expect(
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of these last two assertions? My initial impression (might be missing something though) is that if the enrolment is removed from the DB that's us covered - the rest is just implementation details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is in the delete logic that is why I added them

   await enrollments(db).delete(enrollmentKey);
    await events(db).delete(enrollmentKey);
    await tasks(db).delete({
      name: `trigger:${enrollmentKey.course_id}:${enrollmentKey.username}`,
    });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But yeah there isn't any data on the tables tasks & events

Copy link
Owner

Choose a reason for hiding this comment

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

With a unit test you want to check that the code does all the things you expect it to – so you're looking inside the the app into the implementation. With an integration test, it's more about testing things at the edges e.g. imagine you don't know how the code works, rather you're just looking at it from the perspective of a user (or in this case an API user i.e. the web app).

So for this test, I've just run the web app and looked at how it interacts with the API. I can see that when I create an enrolment, the web app then makes an API request to /courses/js2 and the response contains the field enrollment (which I now realise is spelt incorrectly everywhere hahaha).

When I delete the enrolment, the /courses/js2 endpoint no longer contains that field. This is how the app knows that the user is no longer enrolled on the course.

It can also be be helpful to check if something is stored in the database (for example, when there isn't an API endpoint to reveal the change).

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think these lines are hugely necessary. There is a scenario I can think of where an event is queued, then a user deletes the enrolment before the event is processed. In that case, it would make sense to have those rows deleted. This is an unlikely edge case that I wouldn't worry testing for.

await events(db).delete(enrollmentKey);
await tasks(db).delete({
  name: `trigger:${enrollmentKey.course_id}:${enrollmentKey.username}`,
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With a unit test you want to check that the code does all the things you expect it to – so you're looking inside the the app into the implementation. With an integration test, it's more about testing things at the edges e.g. imagine you don't know how the code works, rather you're just looking at it from the perspective of a user (or in this case an API user i.e. the web app).

So for this test, I've just run the web app and looked at how it interacts with the API. I can see that when I create an enrolment, the web app then makes an API request to /courses/js2 and the response contains the field enrollment (which I now realise is spelt incorrectly everywhere hahaha).

When I delete the enrolment, the /courses/js2 endpoint no longer contains that field. This is how the app knows that the user is no longer enrolled on the course.

It can also be be helpful to check if something is stored in the database (for example, when there isn't an API endpoint to reveal the change).

I don't understand your point
Do you mean I should have tested the exact response from get /courses/js2 & delete /courses/js2 instead of checking if the enrollment table has the user & course details or not ?

Copy link
Owner

Choose a reason for hiding this comment

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

Not exactly. Yes, check the response e.g. status code. But also you want to check if the enrolment was in fact deleted. Did the intended effect take place?

How to do that? Check in database – one option. But then we're looking at the internals of the app. The more we inspect the internals the more brittle the tests become. What if we switch to DynamoDB? Now the test breaks. Good integration tests allow you to refactor code without having to change the tests.

A better approach is to make another API call to the appropriate endpoint to check that you the user is no longer enrolled. In this way you check that the effect took place, but do so using the interface (the API) that you are testing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is clear now..Thanks for explaining

jest.config.js Outdated Show resolved Hide resolved
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