Page MenuHomeSoftware Heritage

Client: add API endpoints integration tests
ClosedPublic

Authored by haltode on Jul 31 2019, 2:01 PM.

Details

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 Graph service
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

haltode created this revision.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 ↗(On Diff #6040)

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 ↗(On Diff #6040)

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 ↗(On Diff #6040)

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 ↗(On Diff #6040)

I guess this is not going to stay "1.0" forever. Is there a way we could make that generic?

34 ↗(On Diff #6040)

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 ↗(On Diff #6040)

Just yield "server"

43 ↗(On Diff #6040)

Is there a particular reason to use unittest instead of regular pytest test cases?

46 ↗(On Diff #6040)

Just self.client = client

73 ↗(On Diff #6040)

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 ↗(On Diff #6040)

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 ↗(On Diff #6040)

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 ↗(On Diff #6040)

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 ↗(On Diff #6040)

Right now no, but this can be added easily on the Java part as a CLI optional argument.

haltode updated this revision to Diff 6050.Jul 31 2019, 4:34 PM
  • 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
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 ↗(On Diff #6050)

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 ↗(On Diff #6050)

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'

?

This revision now requires changes to proceed.Jul 31 2019, 4:41 PM
haltode updated this revision to Diff 6052.Jul 31 2019, 5:04 PM
  • Use isinstance instead of type
  • Simplify pathlib paths
  • Use sets to compare stats json keys
haltode marked 2 inline comments as done.Jul 31 2019, 5:04 PM
haltode updated this revision to Diff 6061.Aug 1 2019, 11:36 AM
  • Use any available port for server/client interaction
  • Add version constant in tests
haltode marked 2 inline comments as done.Aug 1 2019, 11:36 AM
seirl accepted this revision.Aug 1 2019, 2:13 PM
seirl added inline comments.
swh/graph/tests/test_api_client.py
23 ↗(On Diff #6061)

Nit: put that in a function maybe? So that it's easier to replace if we put that in swh.core or something.

This revision is now accepted and ready to land.Aug 1 2019, 2:13 PM
haltode updated this revision to Diff 6069.Aug 1 2019, 2:34 PM

Use aiohttp.test_utils.unused_port()

haltode updated this revision to Diff 6071.Aug 1 2019, 2:48 PM

Rebasing on master.

This revision was automatically updated to reflect the committed changes.