Upgrade for CKAN 2.9/python 3 #4
Conversation
we only care about using/upgrading the import plugin
fixes ValueError: Unicode strings with encoding declaration are not supported. Please use bytes input or XML fragments without declaration. lxml prefers to be passed bytes because xml files will include an encoding declaration in the header e.g: `<?xml version='1.0' encoding='UTF-8'?>`
| return open(os.path.join(os.path.dirname(__file__), 'test_data', filename), 'rb') | ||
|
|
||
|
|
||
| def _patch_storage_path(monkeypatch, tmpdir, ckan_config): |
There was a problem hiding this comment.
Unforuntately I wasn't able to do this in setup() because it doesn't get passed the vars monkeypatch, tmpdir and ckan_config so I am just calling _patch_storage_path(monkeypatch, tmpdir, ckan_config) at the start of every test that needs it. Can you think of any way round this that would allow us to do this in setup()?
There was a problem hiding this comment.
What about creating your own patch_storage fixture that does this and applying it to the class alongside clean_db etc?
There was a problem hiding this comment.
OK. I think I'm going to punt this job to an issue
| ): | ||
| _patch_storage_path(monkeypatch, tmpdir, ckan_config) | ||
| # TODO: test importing from URLs, mock the HTTP requests with responses | ||
| """ |
There was a problem hiding this comment.
As noted in the top post, there's a bunch of stuff I haven't tested, but I didn't want this to turn into too much of a timesink. I have to stop pulling this thread somewhere. At least having got some tests written and running under CI we've got a starting point to improve from if we want to..
There was a problem hiding this comment.
Sounds good, I think what we have now is comprehensive enough.
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
=========================================
Coverage ? 82.67%
=========================================
Files ? 7
Lines ? 554
Branches ? 0
=========================================
Hits ? 458
Misses ? 96
Partials ? 0 Continue to review full report at Codecov.
|
|
|
||
| Make sure | ||
|
|
||
| * [ckanext-harvest extension](https://github.com/ckan/ckanext-harvest) and |
There was a problem hiding this comment.
Do we need ckanext-harvest? You've deleted the harvesters right?
There was a problem hiding this comment.
Although we have removed the harvester plugin, internally the DdiImporter module (which use to perform the import, even when initiated via the frontend) is still an implementation of HarvesterBase
ckanext-ddi/ckanext/ddi/importer/ddiimporter.py
Lines 8 to 19 in 0364463
| return open(os.path.join(os.path.dirname(__file__), 'test_data', filename), 'rb') | ||
|
|
||
|
|
||
| def _patch_storage_path(monkeypatch, tmpdir, ckan_config): |
There was a problem hiding this comment.
What about creating your own patch_storage fixture that does this and applying it to the class alongside clean_db etc?
| ): | ||
| _patch_storage_path(monkeypatch, tmpdir, ckan_config) | ||
| # TODO: test importing from URLs, mock the HTTP requests with responses | ||
| """ |
There was a problem hiding this comment.
Sounds good, I think what we have now is comprehensive enough.
| @@ -21,7 +22,7 @@ Features: | |||
|
|
|||
There was a problem hiding this comment.
Worth updating the features list above ^ as most of these have been removed here
There was a problem hiding this comment.
Good catch 👍 I've pushed some more readme updates
This PR has ended up quite big and sprawling, but I think I've done a pretty decent job of making each commit a sensible unit to review at least. High-level things going on in this PR: