Implement empty response #1
Conversation
| web: | ||
| build: | ||
| context: .. | ||
| dockerfile: docker/Dockerfile.dev |
There was a problem hiding this comment.
Can we match https://github.com/libero/browser/blob/master/Dockerfile? So prod -> test -> dev.
There was a problem hiding this comment.
If I'm reading this correctly, all of these images will be created when referencing this file. Is that correct? I don't see the advantage of having everything in one file. Could you explain please?
There was a problem hiding this comment.
Could leave the test and prod image for later when we setup a pipeline
There was a problem hiding this comment.
Ok. Was just bit concerned that this looked like it would be a dev-only container.
There was a problem hiding this comment.
It is at the moment (doesn't have the code), but it should be transformed into a prod container derivative as soon as the prod container image is there
tests/test_search.py
Outdated
| response = client.get('/search') | ||
| assert response.status == '200 OK' | ||
| assert response.data == (b'<?xml version="1.0" encoding="UTF-8"?>' | ||
| b'<item-list xmlns="http://libero.pub"/>') |
There was a problem hiding this comment.
Would prefer testing the structure.
There was a problem hiding this comment.
can you provide an example please?
There was a problem hiding this comment.
Some equivalent of https://phpunit.readthedocs.io/en/7.4/assertions.html#assertxmlstringequalsxmlstring, so makes sure that two XML documents are the equal rather than two strings being the same.
There was a problem hiding this comment.
So the example you have linked to is actually testing against two strings and the function is entitled assertxmlstringequalsxmlstring which suggests the same.
In the test, response.data is the actual data. What it's being checked against looks like a string but is a string representation of the byte data, hence the little b at the beginning. I'm not aware of a better way to represent this in python but I'm open to suggestions.
There was a problem hiding this comment.
>>> from lxml import etree
>>> etree.fromstring(b'<?xml version="1.0" encoding="UTF-8"?><item-list xmlns="http://libero.pub"></item-list>').getchildren() == []
True
but the syntax may vary depending on the XML library being used.
There was a problem hiding this comment.
@giorgiosironi I see. So what is missing from the test is checking that item-list doesn't have child elements?
There was a problem hiding this comment.
So the example you have linked to is actually testing against two strings and the function is entitled
assertxmlstringequalsxmlstringwhich suggests the same.
Strings of XML, so it recognises <item-list xmlns="http://libero.pub"></item-list> and <item-list xmlns="http://libero.pub"/> as equal.
There was a problem hiding this comment.
@thewilkybarkid oh I see. I totally missed that. I'll see what I can do.
There was a problem hiding this comment.
I have reimplemented the xml generation and parsing using the lxml library.
docker/Dockerfile.dev
Outdated
| ENV PYTHONUNBUFFERED=1 | ||
| ENV PYTHONDONTWRITEBYTECODE=1 | ||
| ENV PATH "/root/.poetry/bin:$PATH" | ||
| RUN curl -sSL https://raw.githubusercontent.com/sdispater/poetry/master/get-poetry.py | python |
There was a problem hiding this comment.
Is there a (versioned) container that can be used?
There was a problem hiding this comment.
I think this is inferred in the pyproject.toml file.
There was a problem hiding this comment.
I'm referring to Poetry itself, like https://hub.docker.com/_/composer.
There was a problem hiding this comment.
I see. There are some floating around on Docker Hub but not an official image.
There was a problem hiding this comment.
From what we have seen yesterday, no luck: have to install Poetry on top of a python official image
| @@ -0,0 +1,19 @@ | |||
| [flake8] | |||
There was a problem hiding this comment.
It'd be good to have a configured coding-standard checker as a dependency, this was started in https://github.com/elifesciences/proofreader-python.
We have https://github.com/libero/php-coding-standard which is applied across all the PHP repos and saves having to try and replicate the same configuration everywhere. It does only do the one tool though, but is the one that needs the fine-grained control.
There was a problem hiding this comment.
Good idea. I would like to see how this project evolves and, once well established, move the coding standard checks to its own repo. A separate issue should be created for this though and prioritised accordingly.
|
Two other things we need to do:
(See also https://github.com/libero/api-problem-bundle and https://github.com/libero/ping-controller.) |
|
@thewilkybarkid implementing |
| assert response.content_type == 'application/xml; charset=utf-8' | ||
| assert xml.docinfo.encoding == 'UTF-8' | ||
| assert xml.docinfo.xml_version == '1.0' | ||
| assert xml.docinfo.root_name == 'item-list' |
There was a problem hiding this comment.
probably don't need this as it's checked in the next line, but doesn't matter much
|
|
||
| from lxml import etree | ||
|
|
||
| LIBERO_NAMESPACE = 'http://libero.pub' |
There was a problem hiding this comment.
should this use http or https? spec says http but thought I would double check.
There was a problem hiding this comment.
These are not URLs that will actually be loaded, they only serve the purpose to avoid clashes between different organizations that could choose the same namespace:
https://en.wikipedia.org/wiki/XML_namespace#Namespace_names
docker/Dockerfile.dev
Outdated
| apk del .build-dependencies | ||
|
|
||
| COPY ./pyproject.toml ./ | ||
| COPY ./poetry.lock ./ |
There was a problem hiding this comment.
Multiple files can be done in one COPY.
docker/Dockerfile.dev
Outdated
| gcc \ | ||
| musl-dev \ | ||
| && \ | ||
| poetry config settings.virtualenvs.create false && \ |
There was a problem hiding this comment.
can probably push poetry config up too when it is installed? Very minor
docker/Dockerfile.dev
Outdated
| curl -sSL https://raw.githubusercontent.com/sdispater/poetry/master/get-poetry.py | python && \ | ||
| apk del .build-dependencies | ||
|
|
||
| COPY ./pyproject.toml ./ |
There was a problem hiding this comment.
can probably merge these two COPY into one (it's like cp)
| assert xml.docinfo.encoding == 'UTF-8' | ||
| assert xml.docinfo.xml_version == '1.0' | ||
| assert xml.docinfo.root_name == 'item-list' | ||
| assert root.tag == '{http://libero.pub}item-list' # check namespace |
There was a problem hiding this comment.
This all looks rather unsustainable: not that many lines for an empty list, but as soon as there's one with content...
In fact, doesn't actually check that the list in empty?
Still think an equivalent of assertXmlStringEqualsXmlString is a better approach.
There was a problem hiding this comment.
In fact, doesn't actually check that the list in empty?
I suggested a separate test for that case, since this one is oriented to "schema" checks more than content. I do think checking actual listings will be different and this is a test not to be replicated: in the case of a listing we would extract the list of elements with Xpath and check the presence of what we have added but you don't want to write
assert response == \
'<item-list xmlns="http://libero.pub">' \
'<item-ref id="42">' \
'</item-list>'
when you can write:
self.assertFoundItems([42])
or even
self.assertFoundItems([42])
self.assertNotFoundItems([43])
for negative cases and similar.
There was a problem hiding this comment.
Full expected responses have their place to demonstrate a sample of what it should look like, but this is a service which returns a flat list so there's not much explanatory power in replicating the full document for all cases.
Even with JSON you would never write:
assertJsonEquals(items, '[{"key":"value", ...}]')
but the expected value would still be a data structure in the source code, as a string is prone to syntax errors.
There was a problem hiding this comment.
@thewilkybarkid @giorgiosironi I can't find anything like the php assertion method in python. Also, it seems it's not trivial to compare python xml tree objects. lxml has the ability to check a document against a schema. Would this be a better way to test xml?
|
Good for me 👍 |
|
Ok. I'll merge for now and happy to revisit individual aspects. |
Forked
masterinto a branch, create a newmasterwith justREADME.mdand merged it into this branch to make sure they have a common ancestor.