-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade to Django v5.1.x #2115
Comments
|
The blocker here is primarily the coding system tests:
There's also a related Django warning that we could ignore for now:
There's a warning against accessing the database in this way in the Django documentation. Here's the relevant PR where that coding system versioning code was introduced: #1397. |
This is the relevant part of that PR's comment:
|
More digging and I think that:
|
So Django 5.0.10 does pass the tests. You still get the warning:
but I don't think that's necessary to fix to get the Django 5.1 upgrade to work. It may be that this gets blocked by Django in future and we're forced to fix it. I opened #2289 for this, because I believe it's separate. That possibly leaves the configuration for the coding systems tests as being the blocking issue._ ⚠ Edit: maybe not, and maybe it's still related? The documentation on the test database also warns against this use of |
Here's the relevant Django PR that made the change for Django 5.1: django/django#17639. It's definitely this change that's the incompatible change. Reverting it and only that change allows the tests to otherwise pass with Django 5.1. What's odd is that commit refers to "SimpleTestCase". (I'm not sure how pytest-django interacts here, and which TestCase class it's using. I'm also not sure what magic pytest-django is doing to modify the error message, but the failure points to a line of Django code, despite the error message being different.) The behaviour if the database with |
This is a related Slack thread on tests failing, for other reasons. From that, it's worth knowing about
Lines 8 to 15 in 4144caf
That instead then results in complaints like:
This occurs even if just running the Relevant stack trace, showing the failure at migration of the new database.
⚠ Something I noticed: running only There's also info on multi database support in pytest-django's issue tracker: pytest-dev/pytest-django#924. I also tested an older version of pytest-django (4.3.0) and get the same errors. |
This is a complex topic with multiple interlinked issues. Thank you, Steve, for thoroughly investigating, reducing the remaining issues in your draft PR, and tracing the history and references. Below are my thoughts on several areas. Apologies for the length of this message. Happy to discuss any of this if that helps. Overall, I think this has surfaced some issues with testing, implementation, and possibly architecture. We should consider how we respond to these. However, I don't think either main issue should block us from upgrading to Django 5.1 as soon as possible. There appear to be workarounds for the Alternatively, we could initially move to Django 5.0, which doesn’t require immediate workarounds. However, I currently think we can go straight to Django 5.1. We could enquire in Database architecture (separate DB per coding system version)Looking into this issue has made me further question the architecture of having a separate database per coding system version. This approach seems overly complicated, introducing many sharp edges and challenges in understanding and testing, as these issues demonstrate. An alternative could be to structure each coding system as its own app, with a single database per coding system (rather than per version). These apps would add their databases to I haven’t fully thought this through, and these concerns need to be weighed against the rationale for the current design laid out in:
|
In my previous message I suggested that the Steve tried:
I think that won't work for a full dynamic DB import test because if we add those databases, they would be initialized by the test framework. This would cause the test to fail, as it expects the database file and its schema/content not to be present until they are explicitly created during the test. We could try and remove the database the framework creates but that seems messy. Why is the check failing? #17639 and #17879 added the checks to
This is in a patched extension to There are three more conditions we could consider fixing or patching to pass these checks.
Finally, |
Mike's explanation above is an excellent overview at the multiple issues that the change in behaviour of Django 5.1 exposes in our code and tests. For quick reference, I've summarised the main problems, and the possible solutions Mike proposed, so that we can see at a glance when reviewing them. We may want to open separate issues for the problems that do not block the Django 5.1 upgrade, but I've summarised them here too initially.
|
The other consideration is that, aside from "we eventually will have to upgrade to a version of Django with this breaking behaviour" (in April 2026 when the 4.2 LTS is end-of-life), is that we want to upgrade to unblock #2268, which could improve the performance of the site for users. We can then consider the trade-off: is making the tests worse in the short-term — working around them, or making them more hacky, without overhauling the coding system code and tests — worth that benefit? My hunch is probably, yes. In that context, I didn't think Mike's suggested class-based refactoring of the tests looked too bad for a short-term fix. |
With regard to the blocking issue of the test failures, we talked through three options for short-term fixes this afternoon:
|
I couldn't find a way to make 1 from Steve's latest summary work after our discussion yesterday. I think because the test class is constructed dynamically based on the required set of fixtures, the place you'd need to patch it doesn't have an accessible name at test execution time, that I can see. The test function code is read and its class defined before patches can take effect. Patching the class definition (either in And 3 doesn't work because there isn't a way to return the control flow after the exception is raised. With a Django-style class-based test case as in the So 2, the I noted my concern that modifying
So it's inherits from unittest's
So I think the things it does are: specify the databases to be created at test startup (in a non- So I think my prototype approach in class_based_testcases.diff.txt may be pretty close to something reasonable to use. It does need a proper location for the temporary files, maybe using |
There's a lot here, and I'm not sure I've followed it all, and I maybe missing something. But I wanted to ask a quick question, just to make sure that we're not missing a simpler solution? Why are we considering upgrading to django 5.2? AFAIK, tts not been released yet? Unless I've missed something? I ask, because it looks like the db fixtures changes/breaking is in 5.2, not 5.1, is that right? I would expect that we would need to wait for django-pytest to support 5.2 before we upgraded to it? Apologies if this has already been mentioned! There's a lot to read here. |
The discussion in the tech team meeting clarified that this is still an issue in 5.1, so ignore me. |
Apologies,@bloodearnest, some of my messages mentioned 5.1 vs 5.2 but that was just a thinko for what should have read 5.0 vs 5.1. Edited now. |
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working.
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working.
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working.
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. It was decided that refactoring the existing tests was the solution for now, rather than rethinking them or the code under test entirely. This facilitates the upgrade to Django 5.1 without blocking us further. We may have to revisit this code and these tests in future, with a view to a more comprehensive overhaul. This single class facilitates getting most of the tests working by just moving them into a test class that uses this DynamicDatabaseTestCase class. Those tests have a generalisable pattern for accessing the databases. The BNF and base coding system tests are slightly more complicated, and the issues will be described in those respective subsequent commits. Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. It was decided that refactoring the existing tests was the solution for now, rather than rethinking them or the code under test entirely. This facilitates the upgrade to Django 5.1 without blocking us further. We may have to revisit this code and these tests in future, with a view to a more comprehensive overhaul. This single class facilitates getting most of the tests working by just moving them into a test class that uses this DynamicDatabaseTestCase class. Those tests have a generalisable pattern for accessing the databases. The BNF and base coding system tests are slightly more complicated, and the issues will be described in those respective subsequent commits. Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. It was decided that refactoring the existing tests was the solution for now, rather than rethinking them or the code under test entirely. This facilitates the upgrade to Django 5.1 without blocking us further. We may have to revisit this code and these tests in future, with a view to a more comprehensive overhaul. This single class facilitates getting most of the tests working by just moving them into a test class that uses this DynamicDatabaseTestCase class. Those tests have a generalisable pattern for accessing the databases. The BNF and base coding system tests are slightly more complicated, and the issues will be described in those respective subsequent commits. Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. It was decided that refactoring the existing tests was the solution for now, rather than rethinking them or the code under test entirely. This facilitates the upgrade to Django 5.1 without blocking us further. We may have to revisit this code and these tests in future, with a view to a more comprehensive overhaul. This single class facilitates getting most of the tests working by just moving them into a test class that uses this DynamicDatabaseTestCase class. Those tests have a generalisable pattern for accessing the databases. The BNF and base coding system tests are slightly more complicated, and the issues will be described in those respective subsequent commits. Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. It was decided that refactoring the existing tests was the solution for now, rather than rethinking them or the code under test entirely. This facilitates the upgrade to Django 5.1 without blocking us further. We may have to revisit this code and these tests in future, with a view to a more comprehensive overhaul. This single class facilitates getting most of the tests working by just moving them into a test class that uses this DynamicDatabaseTestCase class. Those tests have a generalisable pattern for accessing the databases. The BNF and base coding system tests are slightly more complicated, and the issues will be described in those respective subsequent commits. Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. Altering this attribute is mentioned in Django's documentation: https://docs.djangoproject.com/en/5.1/topics/testing/tools/#multi-database-support It was decided that refactoring the existing tests was the solution for now, rather than rethinking them or the code under test entirely. This facilitates the upgrade to Django 5.1 without blocking us further. We may have to revisit this code and these tests in future, with a view to a more comprehensive overhaul. The `DynamicDatabaseTestCase class facilitates getting most of the tests working by just moving them into a test class that uses this DynamicDatabaseTestCase class. Those tests have a generalisable pattern for accessing the databases. The tests that need refactoring fall into one of three general patterns: * most tests that do not need a temporary coding system database directory, handled by the `DynamicDatabaseTestCase` class * some tests that need a temporary coding system database directory, handled by the `DynamicDatabaseTestCaseWithTmpPath` class, which extends `DynamicDatabaseTestCase` * a few edge cases that are discussed in subsequent commits; these will be worked around, rather than refactored in this series of commits (though they could be reworked in future): * tests in `coding_systems/base/tests/test_import_data_utils.py` that access the database in fixtures, before the test class' `setUp()` runs; these will be handled in that module specifically * tests in `coding_systems/bnf/tests/test_import_data.py` that uses a pytest fixture to produce import data, instead of fixtures stored in the repository, unlike the other tests Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
See #2115 for full details. The problem this refactoring aims to solve is, in short, as quoted from @mikerkelly in #2115: > The [DatabaseOperationForbidden] errors occur because there's a mismatch > between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > that import databases dynamically are > trying to do and how both the Django and `pytest-django` test case > frameworks work. In a multi-database context, the test frameworks want > to declare which databases are available to a test case at > test-case-definition-time, which then triggers those databases being > created, the test case wrapped in a transaction referencing those > databases, and the databases flushed in clean up. Since 5.1 it's > enforced that test cases only open connections to such databases, to > avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py) > do want to access a database that they do not want to exist at the start > of the test. That's in conflict. So maybe we shouldn't be using test cases > this way at all? Mike proposed several ideas to fix this that we looked at, and we found that: amending the test case attribute that defines the allowed databases seems reasonable, and, further, this was the only solution that we had working. Altering this attribute is mentioned in Django's documentation: https://docs.djangoproject.com/en/5.1/topics/testing/tools/#multi-database-support It was decided that refactoring the existing tests was the solution for now, rather than rethinking them or the code under test entirely. This facilitates the upgrade to Django 5.1 without blocking us further. We may have to revisit this code and these tests in future, with a view to a more comprehensive overhaul. The `DynamicDatabaseTestCase class facilitates getting most of the tests working by just moving them into a test class that uses this DynamicDatabaseTestCase class. Those tests have a generalisable pattern for accessing the databases. The tests that need refactoring fall into one of three general patterns: * most tests that do not need a temporary coding system database directory, handled by the `DynamicDatabaseTestCase` class * some tests that need a temporary coding system database directory, handled by the `DynamicDatabaseTestCaseWithTmpPath` class, which extends `DynamicDatabaseTestCase` * a few edge cases that are discussed in subsequent commits; these will be worked around, rather than refactored in this series of commits (though they could be reworked in future): * tests in `coding_systems/base/tests/test_import_data_utils.py` that access the database in fixtures, before the test class' `setUp()` runs; these will be handled in that module specifically * tests in `coding_systems/bnf/tests/test_import_data.py` that uses a pytest fixture to produce import data, instead of fixtures stored in the repository, unlike the other tests Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
We could upgrade OpenCodelists to the most recent v5.1.x release.
This would make it easier for the automated dependency updater (whether Dependabot or our own in house action) to keep us up-to-date in future.
The text was updated successfully, but these errors were encountered: