Page MenuHomeSoftware Heritage

Refactor the pytest_plugin
ClosedPublic

Authored by douardda on May 6 2020, 5:40 PM.

Details

Summary

This diff consists in a stack of revisions that refactor the pytest_plugin so
that:

  • the kafka_server fixture ensures topics exist on the broker beforehand,
  • the kafka_prefix is used as is everywhere to prevent some naughty code coupling (user of the plugin *must* also modify the kafka_prefix the same way it was done here).

Included revisions are:

  • pytest_plugin: improve error message in consume_messages() helper function
  • pytest_plugin: do not crash if the list of consumed messages is empty

    if assert_all_objects_consumed() helper function.
  • pytest_plugin: modify the kafka_server fixture so topics are created beforehand

    Since we now check that topics exist on the kafka broker when instanciating a JournalClient, topics but be created when the mock kafka broker is started for tests to keep working (e.g. in swh.storage).

    This needs a small adaptation in the JournalClient code itself to ensure "empty" messages used to create topics will be silently ignored.

    The kafka_server() fixture is split in 2 fixtures:
    • kafka_server_base() that only creates the mock kafka server,
    • kafka_server that() uses this later and creates the topics resulting from the conjunction of the kafka_prefix() and the new object_types() fixture.

      This new object_types() fixture is thus used to define the list of topics (in conjunction with the kafka_prefix fixture) a (mock) kafka_server() will create at startup time.
  • test: replace usage of a strategy .example() by a hardcoded value

    to prevent a warning from hypothesis.
  • pytest_plugin: remove '.swh.journal.objects' from test_config fixture

    This induces a tigh coupling with code from other packages that use this fixture (e.g. swh.storage currently have to modify the kafka_prefix resulting from the fixture so the consumer and the writer are in sync).

    If someone wants a customized version of a kafka_prefix, it must be done by overriding the kafka_prefix fixture, not by modifying its returned value.

    Note that this commit will break swh.storage.tests.test_kafka_writer until it is updated.
  • pytest_plugin: use the object_types fixture in test_config

    instead of hardcoding a specific list there that may become out of sync with the default list of topics used elsewhere in the pytest_plugin.

Depends on D3126.

Diff Detail

Repository
rDJNL Journal infrastructure
Branch
better-topics-management
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 12290
Build 18642: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 18641: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D3130 (id=11117)

Could not rebase; Attempt merge onto fa9ab16c30...

Updating fa9ab16..a1afc6a
Fast-forward
 swh/journal/client.py                   |  70 ++++++++-------
 swh/journal/pytest_plugin.py            |  73 ++++++++++++----
 swh/journal/tests/journal_data.py       |   7 +-
 swh/journal/tests/test_client.py        | 148 +++++++++++++++++++++++++++-----
 swh/journal/tests/test_kafka_writer.py  |   3 -
 swh/journal/tests/test_pytest_plugin.py |  51 +++++++++++
 swh/journal/tests/utils.py              |   4 +-
 7 files changed, 280 insertions(+), 76 deletions(-)
 create mode 100644 swh/journal/tests/test_pytest_plugin.py
Changes applied before test
commit a1afc6a05d3e81830091d23165af57b76817b965
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 17:09:50 2020 +0200

    pytest_plugin: use the object_types fixture in test_config
    
    instead of hardcoding a specific list there that may become out of sync
    with the default list of topics used elsewhere in the pytest_plugin.

commit 21f07bedfaf60409119deba09d79be65e7a9a35e
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 17:02:11 2020 +0200

    pytest_plugin: remove '.swh.journal.objects' from test_config fixture
    
    This induces a tigh coupling with code from other packages that use this
    fixture (e.g. swh.storage currently have to modify the kafka_prefix resulting
    from the fixture so the consumer and the writer are in sync).
    
    If someone wants a customized version of a kafka_prefix, it must be done by
    overriding the kafka_prefix fixture, not by modifying its returned value.
    
    Note that this commit will break swh.storage.tests.test_kafka_writer until
    it is updated.

commit b61dae21b80fa089d60e1088195173d01584e856
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 17:00:19 2020 +0200

    test: replace usage of a strategy .example() by a hardcoded value
    
    to prevent a warning from hypothesis.

commit 91534c43139685fc125c824411f71f5a5e550345
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 16:48:22 2020 +0200

    pytest_plugin: modify the kafka_server fixture so topics are created beforehand
    
    Since we now check that topics exist on the kafka broker when instanciating a
    JournalClient, topics but be created when the mock kafka broker is started
    for tests to keep working (e.g. in swh.storage).
    
    This needs a small adaptation in the JournalClient code itself to ensure
    "empty" messages used to create topics will be silently ignored.
    
    The kafka_server() fixture is split in 2 fixtures:
    - kafka_server_base() that only creates the mock kafka server,
    - kafka_server that() uses this later and creates the topics resulting from
      the conjunction of the kafka_prefix() and the new object_types() fixture.
    
    This new object_types() fixture is thus used to define the list
    of topics (in conjunction with the kafka_prefix fixture) a (mock)
    kafka_server() will create at startup time.

commit 31fb9f13b18208cf791f16fe25686124124bdf2e
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 16:42:27 2020 +0200

    pytest_plugin: do not crash if the list of consumed messages is empty
    
    if assert_all_objects_consumed() helper function.

commit d10db19ad78ee204f9c250a3b6beb5881124d4f6
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 16:40:42 2020 +0200

    pytest_plugin: improve error message in consume_messages() helper function

commit ef52b54615b14577d29e8a6257184f72b5df01f2
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue May 5 16:17:24 2020 +0200

    client: replace the hardcoded ACCEPTED_OBJECT_TYPES by kafka introspection
    
    Remove this global variable and check, at instanciation time, that subscribed
    topics exist on the kafka broker.
    
    The semantics of the ``object_types`` argument is thus a bit different:
    if unset (None), the client will subscribe to any existing topic on the kafka
    broker that starts with the prefix.

See https://jenkins.softwareheritage.org/job/DJNL/job/tests-on-diff/66/ for more details.

anlambert added a subscriber: anlambert.

Looks good to me. Just one nitpick, there is some typos in commit messages:

  • instanciation -> instantiation
  • instanciating -> instantiating
swh/journal/client.py
274

can be / cannot be ?

This revision is now accepted and ready to land.May 6 2020, 5:50 PM

Looks good to me. Just one nitpick, there is some typos in commit messages:

  • instanciation -> instantiation
  • instanciating -> instantiating

codespell should run on the commit message too!

several typos reported by anlambert

Build is green

Patch application report for D3130 (id=11120)

Could not rebase; Attempt merge onto fa9ab16c30...

Updating fa9ab16..2533988
Fast-forward
 swh/journal/client.py                   |  70 ++++++++-------
 swh/journal/pytest_plugin.py            |  73 ++++++++++++----
 swh/journal/tests/journal_data.py       |   7 +-
 swh/journal/tests/test_client.py        | 148 +++++++++++++++++++++++++++-----
 swh/journal/tests/test_kafka_writer.py  |   3 -
 swh/journal/tests/test_pytest_plugin.py |  51 +++++++++++
 swh/journal/tests/utils.py              |   4 +-
 7 files changed, 280 insertions(+), 76 deletions(-)
 create mode 100644 swh/journal/tests/test_pytest_plugin.py
Changes applied before test
commit 25339889f89e5d431f8be457983887440ad03f9a
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 17:09:50 2020 +0200

    pytest_plugin: use the object_types fixture in test_config
    
    instead of hardcoding a specific list there that may become out of sync
    with the default list of topics used elsewhere in the pytest_plugin.

commit 957333752e41763d72be37c02c1fc4cf1a833609
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 17:02:11 2020 +0200

    pytest_plugin: remove '.swh.journal.objects' from test_config fixture
    
    This induces a tigh coupling with code from other packages that use this
    fixture (e.g. swh.storage currently have to modify the kafka_prefix resulting
    from the fixture so the consumer and the writer are in sync).
    
    If someone wants a customized version of a kafka_prefix, it must be done by
    overriding the kafka_prefix fixture, not by modifying its returned value.
    
    Note that this commit will break swh.storage.tests.test_kafka_writer until
    it is updated.

commit 302050a6254c02a93e6fa112c4d842dd3d316945
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 17:00:19 2020 +0200

    test: replace usage of a strategy .example() by a hardcoded value
    
    to prevent a warning from hypothesis.

commit 381ceafe5b2d60ef123c3053887276c9e5a93cc3
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 16:48:22 2020 +0200

    pytest_plugin: modify the kafka_server fixture so topics are created beforehand
    
    Since we now check that topics exist on the kafka broker when instantiating a
    JournalClient, topics but be created when the mock kafka broker is started
    for tests to keep working (e.g. in swh.storage).
    
    This needs a small adaptation in the JournalClient code itself to ensure
    "empty" messages used to create topics will be silently ignored.
    
    The kafka_server() fixture is split in 2 fixtures:
    - kafka_server_base() that only creates the mock kafka server,
    - kafka_server that() uses this later and creates the topics resulting from
      the conjunction of the kafka_prefix() and the new object_types() fixture.
    
    This new object_types() fixture is thus used to define the list
    of topics (in conjunction with the kafka_prefix fixture) a (mock)
    kafka_server() will create at startup time.

commit 3e8e7dff4ec5c9acb9c0b17aa60d1c900b20d1d3
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 16:42:27 2020 +0200

    pytest_plugin: do not crash if the list of consumed messages is empty
    
    if assert_all_objects_consumed() helper function.

commit 84e32e6a90e48e05d381848787b70cfde3816a06
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 6 16:40:42 2020 +0200

    pytest_plugin: improve error message in consume_messages() helper function

commit 8ea0de47a79c5b18c555576b64997d8dc81a2071
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue May 5 16:17:24 2020 +0200

    client: replace the hardcoded ACCEPTED_OBJECT_TYPES by kafka introspection
    
    Remove this global variable and check, at instantiation time, that subscribed
    topics exist on the kafka broker.
    
    The semantics of the ``object_types`` argument is thus a bit different:
    if unset (None), the client will subscribe to any existing topic on the kafka
    broker that starts with the prefix.

See https://jenkins.softwareheritage.org/job/DJNL/job/tests-on-diff/68/ for more details.