From 18375532f23deae648ba34709de85cf0a5d5cdd0 Mon Sep 17 00:00:00 2001 From: bkfox Date: Thu, 25 May 2023 11:33:04 +0200 Subject: [PATCH] review code comment --- aircox/models/station.py | 19 ++- aircox/tests/conftest.py | 17 ++- aircox/tests/models/test_station.py | 181 +++++++++++++++++++++------- 3 files changed, 168 insertions(+), 49 deletions(-) diff --git a/aircox/models/station.py b/aircox/models/station.py index 72f0e57..23745bc 100644 --- a/aircox/models/station.py +++ b/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(): diff --git a/aircox/tests/conftest.py b/aircox/tests/conftest.py index f352e75..5c278e0 100644 --- a/aircox/tests/conftest.py +++ b/aircox/tests/conftest.py @@ -8,18 +8,23 @@ from aircox import models @pytest.fixture -def stations(): - return baker.make(models.Station, _quantity=2) +def stations_no_default(): + """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(): - return baker.make(models.Station, default=True) +def station(): + """Default Station.""" + return baker.make(models.Station, default=True, active=True) @pytest.fixture -def station_active(): - return baker.make(models.Station, active=True) +def stations(stations_no_default, station): + return [station] + stations_no_default @pytest.fixture diff --git a/aircox/tests/models/test_station.py b/aircox/tests/models/test_station.py index ef7ee4d..e0558aa 100644 --- a/aircox/tests/models/test_station.py +++ b/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. - # default method : return a selected instance of station but do not save it. - def test_default_without_default_station(self, stations): - for station in stations: - station.default = False - station.save() - assert Station.objects.default() is not None + def test_default_without_default_station(self, stations_no_default): + # we can remove these, since defaults arguments in stations fixture + # for station in stations: + # station.default = False + # station.save() + + # test: `is None` (not: `is not None`) + assert Station.objects.default() is None def test_default_by_pk(self, stations): - for station in stations: - station.default = False - station.save() + # for station in stations: + # station.default = False + # station.save() assert Station.objects.default(station=stations[0].pk) == stations[0] - def test_default_with_default_station(self, stations, station_default): - for station in stations: - station.default = False - station.save() - assert Station.objects.default() == station_default + def test_default_with_default_station(self, station, stations): + assert Station.objects.default() == station - 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() - assert station_active in Station.objects.active() + + Station.objects.bulk_update(stations, ["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( - self, stations, station_default + # rename & param + def test_save_default_station_while_existing_default( + self, station, stations_no_default ): - for station in stations: - station.default = False - station.save() - stations[0].default = True - stations[0].save() - assert Station.objects.filter(default=True).count() == 1 - assert stations[0] in Station.objects.filter(default=True) + # for station in stations: + # station.default = False + # station.save() + obj = stations_no_default[0] + obj.default = False + obj.save() + + # 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: - if ( - port.type == Port.TYPE_ICECAST - and port.direction == Port.DIRECTION_OUTPUT - ): - assert port.__str__() == "output: icecast #1" + # you can make it smaller if you wanna test only one value, + # use iterator and `next` function, or even better... fixture! \o/ + # for port in ports: + # if ( + # port.type == Port.TYPE_ICECAST + # 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