review code comment

aircox/models/station.py

@@ -153,6 +153,21 @@ class Port(models.Model):
         (TYPE_FILE, _("file")),
     )
 
+    TYPE_INPUTS = (
+        TYPE_JACK,
+        TYPE_ALSA,
+        TYPE_PULSEAUDIO,
+        TYPE_HTTP,
+        TYPE_HTTPS,
+    )
+    TYPE_OUPUTS = (
+        TYPE_JACK,
+        TYPE_ALSA,
+        TYPE_PULSEAUDIO,
+        TYPE_ICECAST,
+        TYPE_FILE,
+    )
+
     station = models.ForeignKey(
         Station, models.CASCADE, verbose_name=_("station")
     )
@@ -187,9 +202,9 @@ class Port(models.Model):
         """Return True if the type is available for the given direction."""
 
         if self.direction == self.DIRECTION_INPUT:
-            return self.type not in (self.TYPE_ICECAST, self.TYPE_FILE)
+            return self.type in self.TYPE_INPUTS
 
-        return self.type not in (self.TYPE_HTTP, self.TYPE_HTTPS)
+        return self.type in self.TYPE_OUPUTS
 
     def save(self, *args, **kwargs):
         if not self.is_valid_type():

aircox/tests/conftest.py

@@ -8,18 +8,23 @@ from aircox import models
 
 
 @pytest.fixture
-def stations():
+def stations_no_default():
-    return baker.make(models.Station, _quantity=2)
+    """Non default stations: one active, one inactive."""
+    return [
+        baker.make(models.Station, _quantity=2, active=False, default=False),
+        baker.make(models.Station, _quantity=2, active=True, default=False),
+    ]
 
 
 @pytest.fixture
-def station_default():
+def station():
-    return baker.make(models.Station, default=True)
+    """Default Station."""
+    return baker.make(models.Station, default=True, active=True)
 
 
 @pytest.fixture
-def station_active():
+def stations(stations_no_default, station):
-    return baker.make(models.Station, active=True)
+    return [station] + stations_no_default
 
 
 @pytest.fixture

aircox/tests/models/test_station.py

@@ -5,40 +5,69 @@ from aircox.models import Station, Port
 from aircox.conf import settings
 
 
-@pytest.mark.django_db
+@pytest.mark.django_db(transaction=True)
 class TestStationQuerySet:
-    # default method : not possible to have several stations by default.
+    def test_default_without_default_station(self, stations_no_default):
-    # default method : return a selected instance of station but do not save it.
+        # we can remove these, since defaults arguments in stations fixture
-    def test_default_without_default_station(self, stations):
+        # for station in stations:
-        for station in stations:
+        #    station.default = False
-            station.default = False
+        #    station.save()
-            station.save()
+
-        assert Station.objects.default() is not None
+        # test: `is None` (not: `is not None`)
+        assert Station.objects.default() is None
 
     def test_default_by_pk(self, stations):
-        for station in stations:
+        # for station in stations:
-            station.default = False
+        #    station.default = False
-            station.save()
+        #    station.save()
         assert Station.objects.default(station=stations[0].pk) == stations[0]
 
-    def test_default_with_default_station(self, stations, station_default):
+    def test_default_with_default_station(self, station, stations):
-        for station in stations:
+        assert Station.objects.default() == station
-            station.default = False
-            station.save()
-        assert Station.objects.default() == station_default
 
-    def test_active(self, stations, station_active):
+    def test_active(self, stations):
+        # This is too heavy: multiple db call while we want to avoid as much as
+        #  possible
+        # KISS: keep it simple stupid -> shortest/simplest way
+
+        # for station in stations:
+        #    station.active = False
+        # we can bulk_update this:
+        #   station.save()
+        # database call are made for all loop iteration, while it is not
+        #  necessary (make tests run slower)
+        #   assert station not in Station.objects.active()
+        # assert station_active in Station.objects.active()
+
+        # first solution:
         for station in stations:
             station.active = False
-            station.save()
+
-            assert station not in Station.objects.active()
+        Station.objects.bulk_update(stations, ["active"])
-        assert station_active in Station.objects.active()
+        # comparing `set` is faster and less code
+        station_ids = {s.pk for s in stations}
+        query = Station.objects.active().values_list("pk", flat=True)
+        assert station_ids == set(query)
+
+        # problem: we should add to test values active=False
+        #
+        # second solution:
+        # - we have an inactive station in `stations` fixture.
+        # - we use python `set` in order to ease comparison and checks
+        #   (check TestPortQuerySet.test_active_value_true)
+        active_ids = {s.pk for s in stations if s.active}
+        query = Station.objects.active()
+        ids = set(query.values_list("pk", flat=True))
+        assert ids == active_ids
 
 
-@pytest.mark.django_db
+@pytest.mark.django_db(transaction=True)
 class TestStation:
     def test_stream_field_filled(self, stations):
-        streams_urls = "http://radiocampus.be/stream1.mp3\nhttp://radiocampus.be/stream2.mp3"
+        streams_urls = (
+            "http://radiocampus.be/stream1.mp3\nhttp://radiocampus"
+            ".be/stream2.mp3"
+        )
         stations[0].audio_streams = streams_urls
         assert stations[0].streams == [
             "http://radiocampus.be/stream1.mp3",
@@ -53,6 +82,8 @@ class TestStation:
         for station in stations:
             assert station.name == station.__str__()
 
+    # you can directly take `station` as parameter for your test, this
+    # avoid heavy/harder-to-read code
     def test_save_without_path(self, stations):
         stations[0].path = None
         stations[0].slug = "a-slug-with-dash"
@@ -62,36 +93,59 @@ class TestStation:
             == settings.CONTROLLERS_WORKING_DIR + "\\a_slug_with_dash"
         )
 
-    def test_save_default_station_while_anotherone_is_default(
+    # rename & param
-        self, stations, station_default
+    def test_save_default_station_while_existing_default(
+        self, station, stations_no_default
     ):
-        for station in stations:
+        # for station in stations:
-            station.default = False
+        #    station.default = False
-            station.save()
+        #    station.save()
-        stations[0].default = True
+        obj = stations_no_default[0]
-        stations[0].save()
+        obj.default = False
-        assert Station.objects.filter(default=True).count() == 1
+        obj.save()
-        assert stations[0] in Station.objects.filter(default=True)
+
+        # values_list should return a list with only one item.
+        ids = Station.objects.filter(default=True).values_list("pk", flat=True)
+        assert ids == [obj.pk]
 
 
 @pytest.mark.django_db
 class TestPortQuerySet:
     def test_active_value_true(self, ports):
         for port in ports:
+            # unit test params should always be the same among different runs
+            # which is here not the case because of random.
+            #
+            # same as above with station, you should ensure your fixtures
+            # contains the values you require in different tests, in order to
+            # avoid too much initialization code inside the tests themselves.
+            #
+            # For example, if you have: `ports_active`, `ports_inactive` and
+            # `ports`, this code is not required at all, reduce runtime
+            # overhead.
+            # You can even have a single inactive port, such as you don't have
+            # to test against all items of a list.
             random_value = random.choice([True, False])
             port.active = random_value
             port.save()
+
+        # when you need to check if all items are in a queryset, use the method
+        # `values_list` (returns a list!) to build python `set` that you can
+        # easilly compare with another set.
         active_ports = Port.objects.active(value=True)
         for port in active_ports:
-            assert port.active == True
+            # no need for == True
+            assert port.active
 
     def test_active_value_false(self, ports):
         for port in ports:
             random_value = random.choice([True, False])
             port.active = random_value
             port.save()
+
         for port in Port.objects.active(value=False):
-            assert port.active == False
+            # no '== False'
+            assert not port.active
 
     def test_output(self, ports):
         for port in Port.objects.output():
@@ -104,24 +158,69 @@ class TestPortQuerySet:
 
 @pytest.mark.django_db
 class TestPort:
+    # actually __str__ method are not really important to test, since
+    # it is just to render to user or cli
+    # but still you can do it
     def test__str__(self, ports):
-        for port in ports:
+        # you can make it smaller if you wanna test only one value,
-            if (
+        # use iterator and `next` function, or even better... fixture! \o/
-                port.type == Port.TYPE_ICECAST
+        # for port in ports:
-                and port.direction == Port.DIRECTION_OUTPUT
+        #    if (
-            ):
+        #        port.type == Port.TYPE_ICECAST
-                assert port.__str__() == "output: icecast #1"
+        #        and port.direction == Port.DIRECTION_OUTPUT
+        #    ):
+        #        assert port.__str__() == "output: icecast #1"
+        # -> also the last part '#1' is the port id, so you can't be sure
+        #    it always be '1'.
+        #
+        # Example using iterator and next function, just for the sake of
+        # knowledge
+        type_dir = (Port.TYPE_ICECASE, Port.DIRECTION_OUTPUT)
+        port = next(p for p in ports if (p.type, p.direction) == type_dir)
+        assert str(port) == "output: icecast #{}".format(port.pk)
 
+    # is_valid_type:
+    # - i've added constants on Port model: to make code more clear and
+    #   reusable
+    # - this also reduce tests complexity
+    #
+    # Example (you might should adapt):
+
+    def test_is_valid_type_for_input(self, input_port):
+        # lets just take one, it should be sufficient
+        input_port.type = Port.TYPE_INPUTS[0]
+        assert input_port.is_valid_type()
+
+    def test_is_valid_type_for_input_is_false(self, input_port):
+        # lets just take one, it should be sufficient
+        input_port.type = next(
+            t for t, _ in Port.TYPE_CHOICES if t not in Port.TYPE_INPUTS
+        )
+        assert not input_port.is_valid_type()
+
+    # to write also for output ports
+
+    # -- you can remove thoses is_valid_type tests then
     def test_is_valid_type_and_type_is_input(self, ports):
         for port in ports:
             if port.direction == Port.DIRECTION_INPUT:
-                assert port.is_valid_type() == True
+                # no need to '== True', just assert as below example
+                # usually when you test for the trueness of an obj:
+                # - testing true: assert port.is_valid_type()
+                # - testing false: assert not port.is_valid_type()
+                # check also different between `==` and `is` comparison
+                # operators
+                # assert port.is_valid_type() == True
+                assert port.is_valid_type()
 
     def test_is_valid_type_and_type_is_not_input(self, ports):
         for port in ports:
             if port.direction == Port.DIRECTION_OUTPUT:
-                assert port.is_valid_type() == True
+                assert port.is_valid_type()
 
+    # you can run here only two tests:
+    # - one for a valid_type
+    # - one for an invalid type
     def test_save_with_valid_type_icecast_output(self, ports):
         assert Port.objects.get(id=ports[0].id) is not None