Page MenuHomeSoftware Heritage

swh-journal: Add tests to the journal publisher using kafka instance
ClosedPublic

Authored by ardumont on Mar 13 2019, 3:31 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Commits
rDJNLcfca42e7f465: tox.ini: Add the pre-download kafka deps step
rDJNL5c8779fd3bb6: test_publisher2: Start trying to test with kafka
rDJNLb70f7d2218a1: tests: Simplify initialization test to use fixtures consistently
rDJNLb66ad089f89b: Install kafka dependencies for test purposes
rDJNL0aa8c5e2c7b4: bin/install-kafka.sh: Fix indentation
rDJNL71f4f5fec873: test_publisher_kafka: Rename it to be nicer in the diff
rDJNL143f9ce79341: tox: Pass specific environment variable to tox
rDJNL090640f0d27d: test_publisher: Factorize behavior to allow easier new testing
rDJNL194ce5686cfb: setup.py: Get back to initial formatting
rDJNL77cb658ad6b1: test_publisher: Avoid repetition
rDJNL617b0d4af0cd: bin/install-kafka.sh: Regroup variable together
rDJNLbdae1997dbfb: publisher: Rename tests 2/2
rDJNL8d556630f787: tox.ini: Remove pifpaf
rDJNL6e6fc773ddba: Makefile.local: Add a target to clean up cache artifacts
rDJNLc59be21ee62b: test_publisher2: Progress towards actual publisher test with kafka
rDJNL240243d53722: tests: Inline the kafka_consumer pytest fixture
rDJNL1bce33eb94b6: test_publisher: Reorganize code
rDJNL68fe907f79ec: Remove spurrious print statements
rDJNL7bfc45ca930a: test_publisher2: Make test work in tox!
rDJNL3a4238880ba8: test_publisher2: Make the test pass
rDJNL202cdf1cf5e9: test_publisher: Assert CI is fine with current state
rDJNL1cae7e68ca98: Ignore kafka test dependencies
rDJNL13cc5d8d9b01: publisher: Rename tests
rDJNL6f3b8e55252d: publisher: Add logger
rDJNLf56bb266fbb8: test_publisher2: Fix publisher initialization
rDJNL73638b6493ce: setup.py: Use a shell script to ease the kafka development setup
rDJNL835f953267f1: setup.py: Install kafka alongside tests
rDJNLbdaef3fed36b: test_publisher: Use direct instantiation of the in-memory storage
rDJNL0e17f6cb5973: README: Explain how to run tests
rDJNL5f14837e9120: test_publisher2: Explain why we need the transformation
rDJNLbbd1c986478b: test_publisher2: Make the test pass for major object types
rDJNLee1ae96cf051: setup.py: Add check on tarball fetch and separate clean up command
rDJNLe49fc8543f5d: Remove unused import
rDJNL26b9eb434aee: publisher: Add config checks within the publisher
rDJNLfa53bac231c3: Bootstrap publisher tests with kafka instance
rDJNLf32415142b0d: conftests: Explicit that the broker is changed during fixture init
rDJNLb8cd821574fa: tests/conftest: Ensure failure message if inexisting kafka install
rDJNL319f9c135781: tox.ini: Remove unused dependency
Summary

Testing the publisher with a real kafka instance through the pytest-kafka dependency.

This also:

  • documents with a script how and what to install in regards to the kafka needed
  • extracts the configuration checks done on the publisher via the cli and push those checks in the publisher directly.
  • those checks are now tested (they helped in the debugging process of those tests)
  • update the readme

Related T1276
Depends on D1280
Depends on D1282

Test Plan

tox

Diff Detail

Repository
rDJNL Journal infrastructure
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ardumont added a comment.EditedMar 14 2019, 7:35 PM

Will remove the 'develop' step later

In the mean time:

  • tox.ini: Remove unused dependency
  • publisher: separate pytest test from mocked ones
  • test_publisher: Reorganize code
  • publisher: Initiate the journal cli to start running the publisher
  • test_publisher2: Start trying to test with kafka
  • test_publisher: Use direct instantiation of the in-memory storage
  • test_publisher2: Progress towards actual publisher test with kafka
  • test_publisher2: Make the test pass
  • test_publisher2: Explain why we need the transformation
ardumont planned changes to this revision.Mar 14 2019, 7:36 PM
ardumont updated this revision to Diff 3949.Mar 15 2019, 1:54 PM

Rebase on latest master (and removes the cli adaptation part, D1249)

ardumont planned changes to this revision.Mar 15 2019, 1:54 PM
ardumont updated this revision to Diff 3981.Mar 18 2019, 7:33 PM

Make test work for major object types

Build has FAILED

Because flake8 is missing on the job node somehow.

ardumont planned changes to this revision.Mar 18 2019, 7:39 PM

I think i'm done for the main part of the tests.

Now i need to concentrate on removing the part about the target ./setup.py develop.
Well not necessarily removing that part (because that could prove useful for others) but at least not make the ci job rely on it (through tox in the current diff's state).

ardumont updated this revision to Diff 4000.Mar 19 2019, 10:55 AM

Rebase on latest master

ardumont planned changes to this revision.Mar 19 2019, 10:55 AM
ardumont updated this revision to Diff 4001.Mar 19 2019, 11:40 AM

Remove unused import

ardumont planned changes to this revision.Mar 19 2019, 11:41 AM
ardumont edited the summary of this revision. (Show Details)Mar 19 2019, 11:48 AM
ardumont retitled this revision from wip: pytest-kafka: ci tryout to swh-journal: Add tests to the journal publisher using kafka instance.Mar 20 2019, 8:55 AM
ardumont edited the summary of this revision. (Show Details)
olasd requested changes to this revision.Mar 20 2019, 10:21 AM

So, if we keep the setup.py snippet to install kafka, I think it should be a completely separate subcommand of setup.py, rather than overriding the default behavior of the develop subcommand, which has a somewhat standard meaning of "install the current module for a development setup".

But I really think those 50 lines of python should just be turned into 5-10 lines of shell script that would be more readable and do the exact same thing.

(disclaimer: I still haven't looked too closely at the actual tests)

So, if we keep the setup.py snippet to install kafka, I think it should be a completely separate subcommand of setup.py, rather than overriding the default behavior of the develop subcommand, which has a somewhat standard meaning of "install the current module for a development setup".

well, it was for that indeed, for the tests to work so for development.

(I don't know yet how to make a subcommand of setup.py, i just started from the pytest-kafka's intial setup.py which does that)

But I really think those 50 lines of python should just be turned into 5-10 lines of shell script that would be more readable and do the exact same thing.

fine by me, i had it in D1280 (prior to removing it) so i'll just take it back for here.

(disclaimer: I still haven't looked too closely at the actual tests)

ack, that's fine ;)

For information, it's just:

  • write to the publisher (as our swh.storage.listener does)
  • publisher reacts, reifies objects and publish them
  • a consumer of the publisher asserts the objects are the expected reified ones
ardumont updated this revision to Diff 4033.Mar 20 2019, 2:06 PM
  • README: Explain how to run tests
  • setup.py: Use a shell script to ease the kafka development setup
  • Makefile.local: Add a target to clean up cache artifacts

This will break test as the tox has been adapted to remove the
dependency on setup.py develop. This will be addressed when D1280,
D1282 converge and land.

ardumont updated this revision to Diff 4034.Mar 20 2019, 2:20 PM
  • bin/install-kafka.sh: Fix indentation
ardumont updated this revision to Diff 4035.Mar 20 2019, 2:22 PM
  • bin/install-kafka.sh: Fix indentation for real this time
ardumont updated this revision to Diff 4036.Mar 20 2019, 2:32 PM
  • bin/install-kafka.sh: Fix indentation

After fixing my emacs setup:

;; default indentation setup
(setq-default indent-tabs-mode nil
              tab-width 4)
ardumont updated this revision to Diff 4050.Mar 22 2019, 3:49 PM
  • tests/conftest: Ensure failure message if inexisting kafka install
  • tox: Pass specific environment variable to tox
  • setup.py: Get back to initial formatting
ardumont updated this revision to Diff 4051.Mar 22 2019, 3:53 PM

Fix pep8 violation

installed back the git-pre-commit hook to avoid that again

ardumont updated this revision to Diff 4054.Mar 22 2019, 4:11 PM
ardumont added a subscriber: douardda.
  • bin/install-kafka.sh: Regroup variables together

Following oral discussion with @douardda

ardumont updated this revision to Diff 4055.Mar 22 2019, 4:14 PM
  • test_publisher_kafka: Rename test_publisher to make a nicer/smaller diff
ardumont updated this revision to Diff 4075.Mar 25 2019, 11:33 AM

Rework commit messages

Aside from that, nothing changed

ardumont updated this revision to Diff 4081.Mar 25 2019, 3:50 PM
  • tests: Inline the kafka_consumer pytest fixture
  • tests: Simplify initialization test to use fixtures consistently
This revision was not accepted when it landed; it landed in state Needs Review.Mar 25 2019, 3:53 PM
This revision was automatically updated to reflect the committed changes.