ponder-lab / Imperative-DL-Study-Web-App

0 stars 0 forks source link

fail the test if there is an exception thrown with integrity error #167

Closed ZhongweiL closed 2 years ago

ZhongweiL commented 2 years ago

Intends to fix the build error by catching the integrity error if there is one and fail the test.

khatchad commented 2 years ago

This sounds like you are sweeping the problem underneath the rug instead of fixing it. Why are we getting an integrity error?

ZhongweiL commented 2 years ago

The integrity error is part of the test and the test should fail if it happens. The error is from the constraint we have on the 'name' field, which we removed in the new version of the model. The test won't pass until the most recent changes are migrated to the db.

khatchad commented 2 years ago

The integrity error is part of the test and the test should fail if it happens.

I don't think so. Why are we expecting an integrity error? What is the error we are seeing? Again, my guess is that you are receiving this error because you haven't removed everything that needs to be removed from the DB in the proper order at the start of the test. Although, you could argue that your tests should be given a fresh DB to work with ...

The error is from the constraint we have on the 'name' field, which we removed in the new version of the model.

Can we see the error message?

The test won't pass until the most recent changes are migrated to the db.

If that is the case, where is the PR with the migration updates?

ZhongweiL commented 2 years ago

The error message can be seen in the build summary, it's MySQLdb._exceptions.IntegrityError: (1062, "Duplicate entry 'John Smith' for key 'name'"), it's happening because I created a categorizer entry with the name 'John Smith' right before this, and it happened because the db is not allowing entries to have the same name, which is the whole point of the test.

khatchad commented 2 years ago

That makes sense, but your test code is still incorrect. If you are expecting an exception to occur, you must use the correct API that says, "I am expecting an exception to be thrown if this kind. "

khatchad commented 2 years ago

https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises

khatchad commented 2 years ago

The title of this PR should be pass the test (not fail it). It's already failing. We need it to pass.

ZhongweiL commented 2 years ago

In this case, we are expecting no exception to occur. Since it occurred, the test should fail. There is no assertion for no exception, so we would use a try catch block according to this post

ZhongweiL commented 2 years ago

You wanted duplicate names to be allowed, right?

khatchad commented 2 years ago

In this case, we are expecting no exception to occur. Since it occurred, the test should fail.

Oh, we are expecting no exception? Then, yes, the test should fail given the current state of the code. But, the code should be fixed so that the test passes.

khatchad commented 2 years ago

You wanted duplicate names to be allowed, right?

Ah, is that what we said, that duplicate names are OK but the initials must be unique? In that case, why are duplicate names prohibited?