Add testing and remove unused code #257

VirginiaDooley · 2024-09-26T15:07:12Z

Ref https://app.asana.com/0/1204880927741389/1208159470102409/f

Added Playwright tests for the front-end leaflet upload process.
Replaced TestCase with pytest for testing.
Introduced YNR API key to bypass rate limiting.
Resolved security vulnerability by upgrading Django from 4.2.15 to 4.2.16.
Updated CI configuration to fix persistent issues with Pipenv.
Add overall test coverage
Misc bug fixes

This is needed since the footer links upgrade

chris48s · 2024-11-27T11:54:27Z

OK. There's a lot going on this this PR and it is a bit hard to grok it all and comment usefully. I have a real mess of questions, feedback and future work. But I will try and say some useful things.

1. Testing

One of the objectives here is to get the code we care about under test to support future modifications.

Here's my experience of checking the code out and running the tests locally:

My first instinct was to run pipenv run test, which exists but doesn't run the tests. Can we either ditch that or update it
This is something I've encountered on other projects, but.. on this project, the test settings don't pull in your local settings. This is good IMO because then you get consistent test runs even if you're fiddling with your local settings. But how are you setting your DB connection details for test? I copied my DATABASES declaration from local.py to testing.py to get moving, but I reckon I am missing a trick here. How are you doing this?
Having got the test suite to run, I have 3 tests failing with AssertionError: Found browser console output: Failed to load resource: the server responded with a status of 404 (Not Found): {'url': 'http://localhost:58045/static/app.js', 'lineNumber': 0, 'columnNumber': 0}. I tried running ./manage.py collectstatic to see if that fixed it, but same error. Any idea what I am missing? I have not been able to get a complete test run locally.

I'll leave some more comments inline on the diff about testing.

2. Removing unused code

One of the objectives of this PR is to ditch a bunch of code we are not using to make the code easier to work on and reason about. It is always a big productivity sink when you spend hours trying to work out "what does this code do" and the answer turns out to be "nothing". That said, its a slightly nebulous objective to review against because we have not defined up-front what is "used" and "not used". As such, I'm not quite sure how to answer the question "is this finished/right?". That said, there are a number of things I'll pick out that I think might be good to review (and hopefully in some cases delete) under this banner.

The general direction of travel in this PR seems to be getting rid of fixtures and replacing them with factories. However, there are still a number of JSON fixture files knocking about:

These model fields are explicitly marked as being unused

electionleaflets/electionleaflets/apps/constituencies/models.py

Lines 16 to 20 in add000d

    
           # Not used anywhere 
        
           wikipedia_url = models.CharField(max_length=765, blank=True) 
        
           url_id = models.CharField(max_length=300, blank=True) 
        
           guardian_aristotle_id = models.IntegerField(null=True, blank=True) 
        
           guardian_pa_code = models.IntegerField(null=True, blank=True)

Time to bin them?

There are a bunch of files that container either nothing, or nothing of any use that we could easily just delete. Non-exhaustive list:
- https://github.com/DemocracyClub/electionleaflets/blob/master/electionleaflets/apps/elections/tests.py
- https://github.com/DemocracyClub/electionleaflets/blob/master/electionleaflets/apps/elections/views.py
There is an entire app called "legacy". https://github.com/DemocracyClub/electionleaflets/tree/master/electionleaflets/apps/legacy What's the deal with that?
I think there are some dead management commands we can bin. For example, I would posit we are not going to be running these any time soon https://github.com/DemocracyClub/electionleaflets/tree/master/electionleaflets/apps/people/management/commands

Finally, there is some code in core related to running Election Leaflets internationally.

electionleaflets/electionleaflets/apps/core/models.py

Lines 4 to 15 in add000d

    
           class Country(models.Model): 
        
               country_id = models.IntegerField(primary_key=True) 
        
               iso = models.CharField(max_length=6) 
        
               name = models.CharField(max_length=240) 
        
               iso3 = models.CharField(max_length=9, blank=True) 
        
               def __unicode__(self): 
        
                   return self.name 
        
               class Meta: 
        
                   db_table = "country" 
        
                   verbose_name_plural = "Countries"

I think the concept of an election being related to a country other than England/Wales/Scotland/NI is a good candidate for the chop if we're getting rid of stuff we don't need to make our lives simpler.

Again, I will highlight some more points relating to unused code on the diff where it is more appropriate to use inline comments.

If the answer to some of these things is "yes, but not in this PR", that is a fine answer but lets make sure we capture those points.

chris48s · 2024-11-27T11:54:57Z

electionleaflets/settings/testing.py


-MEDIA_ROOT = root("test_media",)  # noqa: F405
+MEDIA_ROOT = mktemp()


Minor, but this creates a new tempdir for every test run and we never clean it up
It would be nice if we could find a way to remove this dir in a post hook after the test suite runs.
Also if we are not using test_media any more, we can clean up the gitignore entries relating to that directory.

Fixed in 16b30c9

electionleaflets/settings/testing.py

chris48s · 2024-11-27T11:55:25Z

electionleaflets/apps/api/feeds.py

-#     def item_description(self, item):
-#         return item.description
-
-
 class ConstituencyFeed(Feed):


We seem to be dropping most of the constituencies app in this PR. Do we definitely want to keep this?

chris48s · 2024-11-27T11:56:03Z

electionleaflets/apps/leaflets/models.py

+from django.core.files import File
+from django.core.files.storage import default_storage
+from slugify import slugify
+
 from constituencies.models import Constituency
 from core.helpers import geocode


Here we are importing the geocode function, but this function is not called anywhere in this file and this is the only place where we are importing it.
I think we can actually completely bin the geocode function, which in turn completely removes the dependency on mapit and any related code.
Unused imports is something a linter could help us catch. More on this later..

chris48s · 2024-11-27T11:56:13Z

electionleaflets/apps/leaflets/tests/data.py

@@ -1,2 +1,291 @@
 MAPIT_POSTCODE_RETURN = """


I think this is already not used anywhere, even before we get rid of geocode()

chris48s · 2024-11-27T11:57:21Z

electionleaflets/apps/api/tests/test_views.py

+from electionleaflets.apps.api.views import LeafletFilter
+from leaflets.models import Leaflet
+from django.utils import timezone
+from uk_political_parties.models import Party


What are your thoughts on the future of this?

At the moment, the uk_political_parties is a bit of a pain point. The package repo itself is abandoned/archived. One of the problems it is causing us is we can't run makemigrations --check in CI because there is a missing migration in that package.

electionleaflets/.circleci/config.yml

Lines 68 to 69 in add000d

# TODO: enable this once we drop uk_political_parties package

#- run: pipenv run python manage.py makemigrations --check

I think I would like us to remove the dependency on that package and bring the handful of things we are using from it (I think just the party and emblem models) inline into this application.

If this one is something to address in a future PR, cool. Lets leave it for now.

chris48s · 2024-11-27T11:57:53Z

electionleaflets/storages.py

+
+    @abc.abstractmethod
+    def save_from_temp_upload(self, source_path, target_file_path):
+        raise NotImplementedError


This is a really minor point, but you don't need to raise NotImplemented in an abstract method. You can just pass

import abc class Parent(abc.ABC): @abc.abstractmethod def save_from_temp_upload(): pass class Child(Parent): pass c = Child()

will throw

TypeError: Can't instantiate abstract class Child without an implementation for abstract method 'save_from_temp_upload'

chris48s · 2024-11-27T11:58:33Z

electionleaflets/apps/leaflets/tests/data.py

+        "post": {
+        "id": "gss:E05013806",
+        "label": "St James's",
+        "slug": "st-jamess",
+        "created": "2022-01-05T11:15:54.795194Z",
+        "last_updated": "2023-07-11T15:31:13.176989+01:00"
+        },


Another very minor point, but something that jumped out at me here is there's some odd indenting going on here.
Lets not get into trying to add it in this PR, but this seems like something that should be being caught by linting/formatting tools which I guess we are not using.

chris48s · 2024-11-27T11:58:44Z

electionleaflets/apps/parties/views.py

@@ -53,6 +32,8 @@ def get_context_data(self, **kwargs):
            | Q(ynr_party_id=f"party:{id}")
        )

+        context["party_name"] = qs.first().ynr_party_name


qs.first() can return None which would cause AttributeError: 'NoneType' object has no attribute ynr_party_name here

chris48s · 2024-11-27T12:00:04Z

Pipfile

+pytest = "*"
+pytest-playwright = "*"
+pytest-cov = "*"
+moto = "*"


One of the things I was hoping we would cover under the banner of "removing unused code[/stuff]" was unused packages. I think there are 2 big problems we have on this project:

I am convinced there is a bunch of stuff in the package tree that we actually don't use at all any more

There is no seperation of prod and dev packages. This is especially problematic for an application deployed to lambda because every time we do a cold boot we are spending time unzipping testing/dev packages that are not used at runtime. Specifically here you are adding playwright, pytest, moto etc into the stuff we have to unzip when we cold boot.

I'd really like us to fix both of these problems.
If the next thing you are going to do is migrate from pipenv to UV, I am happy to just ignore this for now and include it in the UV migration PR. This was one of the things I did as part of that exercise on EE:

Migrate to UV EveryElection#2282

DemocracyClub/EveryElection@6d5d0d1

DemocracyClub/EveryElection@7ad181a

Although the scale of the problem is bigger on this project.

Anyway. If we want to just ignore it for now, fine. Just want to flag it as a thing that needs doing.

symroe · 2024-11-27T15:58:54Z

@chris48s I think this is good for another look. I've made issues (I think) for most of your comments. I've addresses the ones that relate to new code in this PR, and should have fixed your local tests.

symroe · 2024-11-27T16:03:35Z

One thing I'd like to do right before merging this PR is run ruff check --fix and ruff format over the codebase. I want to get that done before doing much else

chris48s · 2024-11-28T14:20:47Z

electionleaflets/apps/parties/views.py

@@ -52,6 +31,10 @@ def get_context_data(self, **kwargs):
            | Q(ynr_party_id=self.kwargs["pk"])
            | Q(ynr_party_id=f"party:{id}")
        )
+        if not qs.exists():
+            raise Http404


I think you need to raise Http404() or Http404("message") here. You can't throw a class

I added some tests for this, because clearly it needed them. I'd previously manually tested. The answer is: better to raise the instance of the exception, but it works either way!

TIL you can throw a class

chris48s · 2024-11-28T14:22:05Z

pyproject.toml

+DJANGO_SETTINGS_MODULE = "electionleaflets.settings.testing"
+python_files = ["test_*.py", "*_test.py"]
+addopts = "--reuse-db --tb=short -p no:warnings"
+django_debug_mode = true


This does fix the tests locally for me 👍
I'm slightly unclear how they worked in CI or for you though. Can you give me a quick explanation of that?

Yes.

pytest-django's live_server runs the server with DEBUG=False. This, mixed with a combination of django-pipeline and whitenoise means that assets are not collected in to static. It's assumed that you've run collectstatic beforehand.

In CI I'm running collectstatic, and you could have got your tests working locally by doing this, however I think it's better if this isn't a required step to get tests running.

The change I made gets pytest-django to turn on DEBUG in live_server, meaning whitenoise will collect static files into static as part of the request that Playwright is making.

Hmm. This is why I'm confused. As I said in the original post

I tried running ./manage.py collectstatic to see if that fixed it, but same error

but lets not get too bogged down in it

Ah right, this is per our conversation this morning: collectstatic --settings electionleaflets.settings.testing isn't the same as not specifying the test settings.

aah that was probably it

chris48s · 2024-11-28T14:27:08Z

Couple of comments inline, one of which needs a small change.
I think given we're punting most of the rest of this to #266 this is probably pretty much good to go, or at least the most valuable next step is to merge it and start working on some smaller/more focussed PRs (and see what pops up in sentry 😬 )

One thing I'd like to do right before merging this PR is run ruff check --fix and ruff format over the codebase. I want to get that done before doing much else

I think I would be in favour of not adding any more stuff to this PR and make a follow up PR where we:

Add a ruff config in pyproject.toml
ruff format
ruff check --fix
Manually fix any errors that require a manual fix (in different commits from the autofixes)
Run ruff in CI build

This is needed because we don't collect static when playwright interacts with the live_server fixture. Without calling collect static we can't serve some static assets, so we can't test that the front end is working. The downside of this is that the live server might act differently when in debug mode. There isn't a lot we can do about this, and we should rely on deplyed integration testing for things like 404s when DEBUG=False.

symroe · 2024-11-29T10:05:14Z

The deploy failed after merging this, due to the Lambda zipfile size being too large. This, in turn is caused by the lack of dev packages (and general package bloat).

I'm not going to try to fix that before moving to uv, as that seems like pointless duplicate work. So I'm ok with this branch not being deployed, as it'll get deployed as part of planned later work.

chris48s · 2024-11-29T12:26:48Z

I figured that might happen

VirginiaDooley changed the base branch from master to upgrades202409-part2 September 26, 2024 15:27

VirginiaDooley force-pushed the upgrades202409-part2 branch from 63d89ee to 4360178 Compare September 26, 2024 15:28

VirginiaDooley changed the base branch from upgrades202409-part2 to master September 26, 2024 15:30

VirginiaDooley changed the base branch from master to upgrades202409-part2 September 26, 2024 15:31

VirginiaDooley force-pushed the hotfix/fix-staging-uploads branch from 1640588 to 8447d57 Compare September 26, 2024 15:38

VirginiaDooley changed the base branch from upgrades202409-part2 to master September 26, 2024 15:39

VirginiaDooley changed the base branch from master to upgrades202409-part2 September 26, 2024 15:53

VirginiaDooley force-pushed the hotfix/fix-staging-uploads branch 2 times, most recently from cc6d803 to e4e2edf Compare September 30, 2024 13:42

VirginiaDooley force-pushed the upgrades202409-part2 branch 5 times, most recently from 21b3792 to 5f8cebb Compare October 1, 2024 12:29

Base automatically changed from upgrades202409-part2 to master October 1, 2024 13:58

VirginiaDooley force-pushed the hotfix/fix-staging-uploads branch 3 times, most recently from 9551458 to 38adb43 Compare October 2, 2024 14:14

VirginiaDooley changed the title ~~Hotfix/fix staging uploads~~ Test frontend with Playwright Oct 2, 2024

VirginiaDooley force-pushed the hotfix/fix-staging-uploads branch 11 times, most recently from 421dfdf to 15c41cd Compare October 8, 2024 21:38

Import switcher in design system

426786a

This is needed since the footer links upgrade

symroe force-pushed the hotfix/fix-staging-uploads branch 2 times, most recently from 412ee7c to 713b4d2 Compare November 20, 2024 20:29

Mock YNR in tests

c8bb940

symroe force-pushed the hotfix/fix-staging-uploads branch 2 times, most recently from b78231e to 67501dc Compare November 25, 2024 15:59

symroe requested a review from chris48s November 25, 2024 17:05

chris48s reviewed Nov 27, 2024

View reviewed changes

symroe mentioned this pull request Nov 27, 2024

Clean up old and unused code #266

Open

chris48s reviewed Nov 28, 2024

View reviewed changes

symroe force-pushed the hotfix/fix-staging-uploads branch from 7ebdbf9 to 5f7e52d Compare November 28, 2024 18:04

symroe added 8 commits November 28, 2024 18:05

Don't add Party objects when saving leaflets

6e1e352

Fail tests if errors are written to the browser console

7d3d0a6

USe test settings in CI and ensure static works

246443d

Save images depending on the backend

e4695fa

Don't set the default postgres user in base settings

fa7264e

Remove test_media and use tempdir as MEDIA_ROOT in tests

29cac4c

Fix Pipfile test command

2da7fd9

symroe force-pushed the hotfix/fix-staging-uploads branch from 5f7e52d to 64c67b1 Compare November 28, 2024 18:05

chris48s approved these changes Nov 28, 2024

View reviewed changes

symroe merged commit a9ee13e into master Nov 29, 2024
4 checks passed

symroe deleted the hotfix/fix-staging-uploads branch November 29, 2024 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add testing and remove unused code #257

Add testing and remove unused code #257

VirginiaDooley commented Sep 26, 2024 •

edited

Loading

chris48s commented Nov 27, 2024

chris48s Nov 27, 2024

symroe Nov 27, 2024

chris48s Nov 28, 2024

chris48s Nov 27, 2024

chris48s Nov 27, 2024

chris48s Nov 27, 2024

chris48s Nov 27, 2024

chris48s Nov 27, 2024

chris48s Nov 27, 2024

chris48s Nov 27, 2024

chris48s Nov 27, 2024

symroe commented Nov 27, 2024

symroe commented Nov 27, 2024

chris48s Nov 28, 2024

symroe Nov 28, 2024

chris48s Nov 28, 2024

chris48s Nov 28, 2024

symroe Nov 28, 2024

chris48s Nov 28, 2024

symroe Nov 28, 2024

chris48s Nov 28, 2024

chris48s commented Nov 28, 2024 •

edited

Loading

symroe commented Nov 29, 2024

chris48s commented Nov 29, 2024


		MEDIA_ROOT = root("test_media",) # noqa: F405
		MEDIA_ROOT = mktemp()

	# TODO: enable this once we drop uk_political_parties package
	#- run: pipenv run python manage.py makemigrations --check

Add testing and remove unused code #257

Add testing and remove unused code #257

Conversation

VirginiaDooley commented Sep 26, 2024 • edited Loading

chris48s commented Nov 27, 2024

1. Testing

2. Removing unused code

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symroe commented Nov 27, 2024

symroe commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris48s commented Nov 28, 2024 • edited Loading

symroe commented Nov 29, 2024

chris48s commented Nov 29, 2024

VirginiaDooley commented Sep 26, 2024 •

edited

Loading

chris48s commented Nov 28, 2024 •

edited

Loading