Details

Reviewers

zack
seirl

Maniphest Tasks

T1920: graph service: add tests for the python client

Commits

rDGRPHc57819ce34ae: client: add API endpoints integration tests
rDGRPH293eb8a57c9f: server: add --port/-p flag option
rDGRPHa098482cba3e: server: switch version numbering from 1.0 to 0.0.1

Summary

Closes T1920.

(python:venv) ↪ pytest
========================================================================== test session starts ===========================================================================
platform linux -- Python 3.5.3, pytest-3.10.1, py-1.8.0, pluggy-0.12.0
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/home/haltode/work/swh/swh-environment/swh-graph/.hypothesis/examples')
rootdir: /home/haltode/work/swh/swh-environment/swh-graph, inifile: pytest.ini
plugins: postgresql-1.4.1, hypothesis-4.32.2, celery-4.3.0, kafka-0.3.1, django-3.5.1, requests-mock-1.6.0
collected 6 items

swh/graph/tests/test_api_client.py ......                               [100%]

======================================================================= 6 passed in 11.69 seconds ========================================================================

Diff Detail

Repository

rDGRPH Compressed graph representation

Branch

client-integration-tests

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 7095
Build 10004: arc lint + arc unit

Event Timeline

haltode created this revision.Jul 31 2019, 2:01 PM

Harbormaster completed remote builds in B7080: Diff 6040.Jul 31 2019, 2:01 PM

seirl requested changes to this revision.Jul 31 2019, 2:37 PM

seirl added inline comments.

swh/graph/tests/test_api_client.py
12	You never seem to use the "server" variable elsewhere in the tests (with good reasons, everything should go through the client, not the process). Maybe you should just yield "client", and rename this whole fixture "graph_client"?
13	Put an root to the path here, even if it's a reference to `os.path.dirname(__file__)`. You don't want to depend on CWD.
13	Also, it looks like you would have an easier time using pathlib here. python from pathlib import Path java_dir = Path(__file__).parent / 'java/server' pom_path = java_dir / 'pom.xml' jar_path = java_dir / 'target/swh-graph-...'
22	I guess this is not going to stay "1.0" forever. Is there a way we could make that generic?
34	Is it possible to give a port parameter to the server? This looks like a recipe for flakiness. Maybe find a random unused port instead, like this? https://docs.aiohttp.org/en/stable/testing.html#aiohttp.test_utils.aiohttp_unused_port
37	Just yield "server"
43	Is there a particular reason to use unittest instead of regular pytest test cases?
46	Just self.client = client
73	You should probably also verify the types of the results here.

This revision now requires changes to proceed.Jul 31 2019, 2:37 PM

seirl added inline comments.Jul 31 2019, 2:39 PM

swh/graph/tests/test_api_client.py
97	Why are we using CountEqual here? Wouldn't it be better to check that the results are exact?

seirl added inline comments.Jul 31 2019, 2:49 PM

swh/graph/tests/test_api_client.py
73	So yeah, check the types of the results that are completely unpredictable, and check the values that you know will stay constant (#edges, #nodes, etc)

haltode added inline comments.Jul 31 2019, 4:20 PM

swh/graph/tests/test_api_client.py
22	I can put it into `__init__` as a constant, but the swh-graph version number should be even more global than tests. I need to look at others swh-* repos to see what is commonly used.
34	Right now no, but this can be added easily on the Java part as a CLI optional argument.

Do not yield unused server internal variable
Use pathlib instead of os.path
Remove unittest test case
Stronger assertions on types and values in test_stats

Harbormaster completed remote builds in B7093: Diff 6050.Jul 31 2019, 4:34 PM

haltode marked 9 inline comments as done.Jul 31 2019, 4:35 PM

seirl added inline comments.Jul 31 2019, 4:39 PM

swh/graph/tests/test_api_client.py
86	Use `isinstance` instead

seirl requested changes to this revision.Jul 31 2019, 4:41 PM

seirl added inline comments.

swh/graph/tests/test_api_client.py
12	Just so that people aren't confused by the parents[3], maybe split into: swh_graph_root = Path(__file__).parents[3] java_dir = swh_graph_root / 'java/server' ?