Closed ZhongweiL closed 2 years ago
@ZhongweiL Are we ready to merge this?
@khatchad Yes.
Didn't we discuss tests?
I was going to create another PR for that, but I can do that in this branch if you want.
@mZneit We can't get the tests to run for this PR but the changes seem good to us. Please review and merge. Thanks,.
@khatchad This seems to be a MySQL issue. Some tests ran when changes were applied to the database directly, but when applying the same changes using django code, they had no effect in MySQL.
Hm. But, since you've merged this change, the tests are passing. Are they running but have no effect?
Ah, actually there are failing. See: https://github.com/ponder-lab/Imperative-DL-Study-Web-App/runs/4377336364?check_suite_focus=true
@ZhongweiL Looks like your test test_same_initials
and others are failing now in the main branch (I still don't know why we couldn't get this running inside the PR, but that is a different problem). Can you see why this is the case? It looks like there's a constraint violation. Please output the SQL command here to debug it. Thanks. Let us know if the test code needs to change or there is a problem with the tested code. Maybe the DB starts with info and you need to delete them in some order?
@khatchad From this answer, it seems like Django wraps each test into a transaction, and Django prevents any ORM operation in the same transaction after an exception is thrown. The code can be fixed by deleting the last two assertions in the function. Another way to solve it is to use the solution in the post which is to limit the scope of the transaction to just that one line, the code would look like this:
try:
with transaction.atomic():
Categorizer.objects.create(name='Jane Scott', initials='JS', user=self.user2)
self.fail()
except IntegrityError:
pass
which will fail the test if the exception was not thrown, and does nothing if it was thrown. But this time the last two assertions can be run because they are not in the same transaction as the categorizer insertion. But I don't think the second solution is necessary unless we really want to keep the last two assertions.
Intends to resolve issue #147 and #132.