Hi, I have a function in context that reads value from other table before inserting a new record. Specifically, it checks class capacity before adding new enrollment into it.
See:
defmodule Enrol.Classes do
def create_enrollment(%{"class_id" => class_id} = attrs \\ %{}) do
# check if class capacity is not exhausted
# and inserts only then, otherwise rolls back
Repo.transaction(fn ->
capacity = Repo.one!(from c in Class, where: c.id == ^class_id, select: c.capacity)
count = Repo.one(from e in Enrollment, where: e.class_id == ^class_id, select: count(e.class_id))
case count < capacity do
true ->
# maybe call insert_enrollment(attrs) from here?
%Enrollment{}
|> Enrollment.changeset(attrs)
|> Repo.insert()
false ->
IO.puts "Class capacity EXCEEDED, rollback"
Repo.rollback(:class_full)
end
end) # end transaction
end
# default generic function
def create_enrollment(attrs \\ %{}) do
%Enrollment{}
|> Enrollment.changeset(attrs)
|> Repo.insert()
end
# alternative (just renamed)
def insert_enrollment(attrs \\ %{}) do
%Enrollment{}
|> Enrollment.changeset(attrs)
|> Repo.insert()
end
end
When writing unit tests for this context, this would mean creating a class fixture first, so that I can read its capacity, before testing insertion of the enrollment. I feel like the unit test should be more granular, and just check whether plain vanilla insertion works.
So my questions are
should I split my function so that it does the checks and then calls the plain vanilla create function?
should I test these two separately? and does it make sense to test each of them?
do I need to rename the second one to insert_enrollment? because they have the same arguments, co calling create_enrollment in the first one would basically result in recursive call and infinite loop, right?
hmmm, your question is more of code structuring, and there’s room for opinion & preference on it, so i would answer just like how i would prefer to do it:
Yes, i generally like to split module which deals with low level data (Enrollment, Classes) with module that govern business logic. in this case you mix actual DB creation logic of enrollment with business logic,
yes, you should test the business logic code part, but testing insert_enrollment is a bit dubious (for me anyway) as it’s really standard code from ecto, i usually trust ecto to do it’s job.
if you do split the module, having create_enrollment would be better as it’s more standard.
just for example, here’s how business logic code might be:
defmodule Enrol.EnrolService do
def enroll_student(student, class) do
Repo.transaction(fn ->
if Classes.class_have_existing_capacity?(class) do
Enrollment.create_enrollment(attrs)
else
Repo.rollback(:class_full)
end
end)
end
Cool, thank you! where would your enroll_student/2 go? If it doesn’t belong to the context, but probably also not to controller, where should it actually be?
I would suggest writing the use cases for your app and translating them into tests. It’s much better to have the unit of the test be a business use case then a context, module, function or a table.
I don’t know the details of your domain and app, so I’m going to make a bunch of assumptions. I’d suggest something like this:
describe "enrolling into a class" do
test "a student can enroll to a class" do
admin = AdminUI.create_admin()
class = AdminUI.create_class(admin)
student = AdminUI.create_student(admin)
enrollment = StudentUI.enroll_to_class(student, class)
assert enrollment.success
end
test "a student cannot enroll if the class is full" do
admin = AdminUI.create_admin()
class = AdminUI.create_class(admin, capacity: 3)
for i <- 1..3 do
student = AdminUI.create_student(admin)
StudentUI.enroll_to_class(student, class)
end
student = AdminUI.create_student(admin)
enrollment = StudentUI.enroll_to_class(student, class)
assert !enrollment.success
end
end
AdminUI and StudentUI are test helpers that model the actions that admins and students can perform in your app. It’s up to you to decide what’s the best way to implement those: you can use the API if you have one, use context or insert straight into the DB if that makes sense.
This way, the tests are going to be disconnected from the implementation and you’ll have much greater freedom in changing things if needed.
I was just wondering whether the logic should be separated from the basic CRUD functions in context, and whether it makes sense to actually test those basic CRUD functions. A generator gives you those functions including tests, so I guess it doesn’t hurt to keep them, I just wasn’t sure whether the context functions are generally to be left untouched, and context-related logic be put into separate modules as suggested above.
The generator is unable to generate anything more useful since it doesn’t have any understanding of the domain.
I have an app where the tests follow that pattern, but the more I look at them, the more I think they are a liability than help. I think I’m going to nuke them.
thank you all for help!
