Feat/faster tests #1007

RoordinkFrank · 2025-10-15T12:07:16Z

branched from #1001

I noticed tests getting from 24s to 8s during pre-assessment as soon as test data transferred to fixtures. Those 8s are with the added pre-assessment tests.

After feedback that this pull request is to big I will try to split this up into different branches. This branch will not change but as other branches are slowly be merged into develop this one should become smaller.

Code with a different branch:

Pre-Assessment tests. Branch is Pre-Assessment tests #1016, not dependent on the branches below.
Proposal tests are faster as a lot of code has been replaced with fixtures. Branch is feat/proposal tests additional fixtures #1009
Review tests are faster as a lot of code has been replaced with fixtures. Branch is Feat/review tests added fixtures #1010.
Test data from review and proposal tests have been merged Branch is feat/merge_proposal&review_test_data #1011. Dependency to "feat/proposal tests additional fixtures" and "Review tests are faster". The two checkboxes just above.
unused code such as generate pdf has been removed Branch is Feat/test cleanup #1012

These branches are branched from each other from top to bottom. To merge test data this is needed.

To clarify, these helper methods are mostly methods that already did exist. Just moved up to parent classes. That or duplicate code refactored to a method.

…done.

…ound other preassessment tests.

… easier this way.

…tion to remake pre-assessement tests in the same way proposal_submisison_tests works.

…out if they are supposed to go wrong or not. Fused testdata from test and proposal_submission_tests.py

…hem. added study fixture, not yet working added messages to assert statement to slowly get more understanding where it goes wrong.

…w, var is no longer needed as naming fixtures for studies temp commented out. Did not get them to work properly

reviews/tests.py

RoordinkFrank · 2025-10-16T08:44:55Z

reviews/tests.py

+
+    def startReview(self):
+        # super().startReview()
+        self.review = start_review(self.pre_assessment)


setUp review -> setUp actual tests -> setUp pre assessment is the order that triggers and it has to remain this way. Yet I do want review -> pre assessment -> actual tests which is imposible in current setup so i have to override startReview here.

Possible solution is the same as in one of my previous comments about proposal inheriting the same was as preAssessment is doing now.

RoordinkFrank · 2025-10-16T09:24:33Z

reviews/tests.py

+        self.assertEqual(
+            reasons[-1],
+            "De onderzoeker geeft aan dat er mogelijk kwesties zijn rondom de veiligheid van de deelnemers tijdens of na het onderzoek.",
+        )


changing self.study.proposal to self.proposal makes it possible to load studies with fixtures. Why this is needed I still do not know, but it works.

…or now.

RoordinkFrank · 2025-10-16T10:19:37Z

reviews/tests.py

-        self.setup_proposal()
        super().setUp()

    def setup_proposal(self):


generate pdf removed here. It was not actually used and significantly slowed down tests as it was done for every test. If it is needed somewhere in the future I propose adding it locally.

… testcases. Autoreview can handle pre assesssment

reviews/tests.py

…t excpect studies it seems.

RoordinkFrank · 2025-10-16T12:33:50Z

proposals/tests/proposal_submission_tests.py

+        self.proposal.save()
+
+    def remove_study(self):
+        Study.objects.filter(proposal=f"{self.proposal.pk}").delete()


Seems to be needed in two tests.

miggol

I understand that this PR is branched from a different PR, but once again this is a PR that addresses various tangentially related things without any issues or stated goals.

As far as I can tell the following distinct things are going on here:

Added fixtures rather than explicit Proposal/user creation.
Merger of Proposal and Review base classes
Other changes/refactoring in SetUp, such as new helper methods
New tests added
Fixes for auto review test and test_long_route (?)
Remove generate PDF

Don't get me wrong, I'm a big fan of the merger of the Proposal an Review testcases in particular. But this borders on impossible to review. For me to Approve this, it will have to be in smaller chunks.

(1) and (2) should be separate PRs into develop
(3) can be dropped for now
(4) should be in this PR, because it adds a test for Feat/pre-assessment_get_revisions which the branch will merge into. That is a good stated goal for this specific PR, stick to that.
If (5) contains fixes for bugs or failing tests, please describe them and make issues for them before fixing in a separate PR. If these fixes are absolutely required for merging (4), I guess they can be in this PR, but in that case please precisely describe what you're doing.
(6) can also be dropped for now, but could be chucked together with (3) in a future PR

miggol · 2025-10-21T16:03:46Z

proposals/tests/proposal_submission_tests.py

+        self.proposal = Proposal.objects.get(
+            reference_number="24-009-01", title="Normal test proposal"
+        )
+        self.pre_assessment = Proposal.objects.get(
+            reference_number="24-010-01", title="Preassessment test proposal"
+        )
+        self.pre_approval = Proposal.objects.get(
+            reference_number="24-011-01", title="Preapproved test pdf"


Just in passing, I must say I disagree with black on this one...

miggol · 2025-10-21T16:10:36Z

proposals/tests/proposal_submission_tests.py

+        self.proposal.save()
+
+    def remove_study(self):
+        Study.objects.filter(proposal=f"{self.proposal.pk}").delete()


Does self.proposal.study_set.all().delete() not work? As written your version seems a bit omslachtig. Also it removes all studies so best to call it remove_studies() (plural)

I would also be down with removing this method entirely. Calling it doesn't even save a single line.

miggol · 2025-10-21T16:19:03Z

proposals/tests/proposal_submission_tests.py

+    def add_supervisor_to_proposal(self):
+        self.proposal.relation = Relation.objects.get(
+            description_nl="als AIO / promovendus verbonden"
+        )
+        self.proposal.supervisor = self.supervisor
+        self.proposal.save()


I don't like that this method implicitly changes the relation. I would rather keep these steps explicit (and in the test) so that if those tests fail, it's easier to see.

RoordinkFrank · 2025-10-24T12:35:14Z

I understand that this PR is branched from a different PR, but once again this is a PR that addresses various tangentially related things without any issues or stated goals.

As far as I can tell the following distinct things are going on here:
1. Added fixtures rather than explicit Proposal/user creation.

2. Merger of Proposal and Review base classes

3. Other changes/refactoring in SetUp, such as new helper methods

4. New tests added

5. Fixes for auto review test and test_long_route (?)

6. Remove generate PDF
Don't get me wrong, I'm a big fan of the merger of the Proposal an Review testcases in particular. But this borders on impossible to review. For me to Approve this, it will have to be in smaller chunks.
* (1) and (2) should be separate PRs into develop

* (3) can be dropped for now

* (4) should be in this PR, because it adds a test for `Feat/pre-assessment_get_revisions` which the branch will merge into. That is a good stated goal for this specific PR, stick to that.

* If (5) contains fixes for bugs or failing tests, please describe them and make issues for them before fixing in a separate PR. If these fixes are absolutely required for merging (4), I guess they can be in this PR, but in that case please precisely describe what you're doing.

* (6) can also be dropped for now, but could be chucked together with (3) in a future PR

After feedback that this pull request is to big I will try to split this up into different branches. This branch will not change but as other branches are slowly be merged into develop this one should become smaller.

Up to date status is at the first comment of this pull request.

RoordinkFrank added 14 commits October 7, 2025 12:15

Added a pre_assesment object in tests. Tests themself have yet to be …

f3f3ef2

…done.

pre assessment has a supervisor test, setup should remain the same ar…

ff68f07

…ound other preassessment tests.

supervisor tests fully work.

63c9519

edo feedback commit.

e6c274f

removed tryexcept, no longer needed.

feb0a7f

moved var to (un)commit BaseReviewTestCase. Hopefully a merge will be…

d5505cf

… easier this way.

Removed pre-assessement tests and restored the original ones. Prepara…

8604b83

…tion to remake pre-assessement tests in the same way proposal_submisison_tests works.

cleanup

7094203

Working review test from fixtures. Still messy

a228bc1

Users to fixtures, test speed goes from 24 to 7s. Possible bug.

c2cd3c6

Added pre assessment tests. Passed ones go wrong, Still have to find …

02197df

…out if they are supposed to go wrong or not. Fused testdata from test and proposal_submission_tests.py

small comment

6766195

added asserts in reasons, went wrong at the wrong statement without t…

61286c0

…hem. added study fixture, not yet working added messages to assert statement to slowly get more understanding where it goes wrong.

Splitpoint branches. Pre assessment and faster tests will be split up

56cbbbb

RoordinkFrank changed the base branch from develop to Feat/pre-assesment_get_revisions October 15, 2025 12:15

RoordinkFrank added 2 commits October 15, 2025 17:03

removed some variables as the description itself is already enough no…

e71dffc

…w, var is no longer needed as naming fixtures for studies temp commented out. Did not get them to work properly

added pass test

416d0c1