fix: polish tests

[SKairinos]

This change is 

[faucomte97]

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)

---

codecov.yml line 6 at r1 (raw file):

  range: 90...90
  status:
    patch:

Following from what Duncan said in the sprint review meeting, should we bring a rule for patch back? To ensure some amount of the added code is tested? But then again, if we just ensure that the overall coverage doesn't decrease, that's also fine (sorry kinda thinking as I type).

Basically, I think the golden rule could be that the overall coverage never goes down. Regardless of how much code was actually covered in the PR itself. Probably easier said than done though.

---

codecov.yml line 9 at r1 (raw file):

      default:
        target: 90%
        threshold: 0%

Do we need this mentioned at all or is the default value something other than 0? I couldn't find a default value mentioned in the docs.

---

codeforlife/user/views/klass_test.py line 87 at r1 (raw file):

        self.assert_get_queryset(
            values=user.teacher.classes,

(Non-blocking comment):

This test has the exact same code as the previous one bar the user getter.
I get it's because in both cases we're using the classes prop which returns either the teacher's class if non-admin or all classes in the school if admin.

This is my question: since we're using the same prop in both tests, in this actually an indication that we should just be testing the classes prop in a model test instead to ensure it returns what we expect, instead of using it in both these model viewset tests? Or, at the end of the day, is it the same thing?

[faucomte97]

not lgtm lols

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)

[SKairinos]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faucomte97)

---

codecov.yml line 6 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Following from what Duncan said in the sprint review meeting, should we bring a rule for patch back? To ensure some amount of the added code is tested? But then again, if we just ensure that the overall coverage doesn't decrease, that's also fine (sorry kinda thinking as I type).

Basically, I think the golden rule could be that the overall coverage never goes down. Regardless of how much code was actually covered in the PR itself. Probably easier said than done though.

I agree. I think we should make it so that the min coverage is 90 and that the coverage never goes down. I don't think we need the patch status check.

---

codecov.yml line 9 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Do we need this mentioned at all or is the default value something other than 0? I couldn't find a default value mentioned in the docs.

Setting this to 0 will means that coverage is allowed to go down by 0%. In other words, the coverage is not allowed to go down. The default value for this null, meaning the coverage can go down by any amount.

---

codeforlife/user/views/klass_test.py line 87 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

(Non-blocking comment):

This test has the exact same code as the previous one bar the user getter.
I get it's because in both cases we're using the classes prop which returns either the teacher's class if non-admin or all classes in the school if admin.

This is my question: since we're using the same prop in both tests, in this actually an indication that we should just be testing the classes prop in a model test instead to ensure it returns what we expect, instead of using it in both these model viewset tests? Or, at the end of the day, is it the same thing?

I think the correct answer is we should test both. We should test that:

1. The model property ".classes" returns the correct queryset
2. The model view set callback "get_queryset()" returns the correct queryset.

I understand your point that these are returning the same queryset so can we not just test one. However, while we know that these two queryset should be the same, we need test that they actually are the same and that something hasn't broken the queryset. This is especially important for future proofing - if any dev changes this code, these tests should assert that the queryset did not break.

[faucomte97]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)

[faucomte97]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)