Differential D3203

Replace MockedJournalClient and MockedKafkaWriter by proper kafka test scaffolding
ClosedPublic
Actions

Authored by douardda on May 29 2020, 3:59 PM.

Details

Reviewers

ardumont

Group Reviewers

Reviewers

Commits

rDSTOf9b2ca3fec14: Replace MockedJournalClient and MockedKafkaWriter by proper kafka test…

Summary

This also kills test_write_replay.py file since it does not test anything
more than what is currently tested in test_replay.py.

Depends on D3202.

Diff Detail

Repository

rDSTO Storage manager

Branch

fix-mocked-kafka

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 12618
Build 19173: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 19172: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	558 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.tests.test_cassandra.TestCassandraStorage::test_revision_log self = <swh.storage.tests.test_cassandra.TestCassandraStorage object at 0x7f4fd4613be0> swh_storage = <swh.storage.validate.ValidatingProxyStorage object at 0x7f4fd45e9208>
	570 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.tests.test_cassandra.TestCassandraStorage::test_revision_log_with_limit self = <swh.storage.tests.test_cassandra.TestCassandraStorage object at 0x7f4ff43a9908> swh_storage = <swh.storage.validate.ValidatingProxyStorage object at 0x7f4fd4774898>
	6 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.fixer::swh.storage.fixer._fix_content
	2 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.fixer::swh.storage.fixer._fix_origin
	4 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.fixer::swh.storage.fixer._fix_origin_visit
		View Full Test Results (2 Failed · 731 Passed · 17 Skipped)

Event Timeline

douardda created this revision.May 29 2020, 3:59 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptMay 29 2020, 3:59 PM

Build is green

Patch application report for D3203 (id=11368)

Could not rebase; Attempt merge onto 6c6080b4d7...

Updating 6c6080b..5dbf353
Fast-forward
 swh/storage/in_memory.py               |   3 +-
 swh/storage/tests/test_replay.py       | 435 +++++++++------------------------
 swh/storage/tests/test_write_replay.py | 112 ---------
 3 files changed, 120 insertions(+), 430 deletions(-)
 delete mode 100644 swh/storage/tests/test_write_replay.py

Changes applied before test

commit 5dbf3539dba6f6da356900413e0f3305a86159cb
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri May 29 14:28:31 2020 +0200

    Replace MockedJournalClient and MockedKafkaWriter by proper kafka test scaffolding
    
    This also kills test_write_replay.py file since it does not test anything
    more than what is currently tested in test_replay.py.

commit eef4900db52a391e20ab51e36bace412ac6730d8
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri May 29 14:21:55 2020 +0200

    Fix InMemoryStorage.origin_visit_upsert() method
    
    the self._origin_visits[origin_url] list was built one element too big
    (since visit ids starts from 1 and not 0).
    
    This is needed to ease writing replayer tests (by comparing these lists).

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

Harbormaster completed remote builds in B12566: Diff 11368.May 29 2020, 4:12 PM

This also kills test_write_replay.py file since it does not test anything
more than what is currently tested in test_replay.py.

Are you sure? replay.py seems to have only fixed examples, while test_write_replay.py uses anything hypothesis generates

ardumont added a subscriber: ardumont.Jun 2 2020, 9:40 AM

ardumont added inline comments.

swh/storage/tests/test_replay.py
128–130	thx clearer ;)
194–197	Adding type would help, at least for the exclude Optional[Set] or something, i guess.
213	what if 'person' is in the exclude parameter (for some reason)? :D

In D3203#78052, @vlorentz wrote:

This also kills test_write_replay.py file since it does not test anything
more than what is currently tested in test_replay.py.

Are you sure? replay.py seems to have only fixed examples, while test_write_replay.py uses anything hypothesis generates

This aspect should be covered with the diff I am preparing in swh.journal, but yes, as is, your remark is true.

douardda added inline comments.Jun 2 2020, 2:14 PM

swh/storage/tests/test_replay.py
213	Well this is a helper function for tests, it is not completely "dumb-proof", indeed, and I do not think we should complexify it for this purpose.

ardumont added inline comments.Jun 2 2020, 3:57 PM

swh/storage/tests/test_replay.py
213	sure

Are you sure? replay.py seems to have only fixed examples, while
test_write_replay.py uses anything hypothesis generates

This aspect should be covered with the diff I am preparing in swh.journal,
but yes, as is, your remark is true.

D3207 started the work indeed. Looks fine to me.

Note that I like the hypothesis approach better though (data generation out of
the model mostly iirc) [1]

[1] I recall discussion whose lasting impression on the conclusion was, we are
not using hypothesis correctly. Aside from that, I don't recall much though
thus why i don't block anything ;)

This revision is now accepted and ready to land.Jun 2 2020, 4:14 PM

ardumont added inline comments.Jun 2 2020, 5:08 PM

swh/storage/tests/test_write_replay.py
48	that's also a current ci build failure... [1] [1] https://jenkins.softwareheritage.org/job/DSTO/job/tests/1214/console

rebased on D3218

Build has FAILED

Patch application report for D3203 (id=11415)

Could not rebase; Attempt merge onto eef4900db5...

Updating eef4900..727eed1
Fast-forward
 requirements-swh-journal.txt               |   2 +-
 requirements-swh.txt                       |   2 +-
 swh/storage/cassandra/converters.py        |   4 +-
 swh/storage/cassandra/storage.py           |   2 +-
 swh/storage/tests/storage_data.py          |  28 +-
 swh/storage/tests/test_api_client_dicts.py |  51 ----
 swh/storage/tests/test_replay.py           | 438 ++++++++---------------------
 swh/storage/tests/test_storage.py          |  17 +-
 swh/storage/tests/test_write_replay.py     | 112 --------
 swh/storage/validate.py                    |  25 +-
 10 files changed, 174 insertions(+), 507 deletions(-)
 delete mode 100644 swh/storage/tests/test_api_client_dicts.py
 delete mode 100644 swh/storage/tests/test_write_replay.py

Changes applied before test

commit 727eed1109938a3366f06ac0c1c5a0426fbd014d
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri May 29 14:28:31 2020 +0200

    Replace MockedJournalClient and MockedKafkaWriter by proper kafka test scaffolding
    
    This also kills test_write_replay.py file since it does not test anything
    more than what is currently tested in test_replay.py.

commit d1bcb9c638d671474c4d5b89d5eaa0869d177419
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Jun 2 17:25:18 2020 +0200

    Adapt to swh.model 0.3
    
    in which List attributes have replaced by Tuple ones.
    
    This requires a bit of adaptation in the code of the
    ValidatingProxyStorage to ensure dict representation of revision
    objects are properly typed.
    
    The test_api_client_dicts.py has been removed since it's not really
    useful any more and would require a fair amount of work to fix it.

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/218/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/218/console

Harbormaster failed remote builds in B12618: Diff 11415!Jun 4 2020, 11:34 AM

rebase

Build is green

Patch application report for D3203 (id=11420)

Could not rebase; Attempt merge onto eef4900db5...

Updating eef4900..f9b2ca3
Fast-forward
 requirements-swh-journal.txt               |   2 +-
 requirements-swh.txt                       |   2 +-
 swh/storage/cassandra/converters.py        |   4 +-
 swh/storage/cassandra/storage.py           |   4 +-
 swh/storage/tests/storage_data.py          |  28 +-
 swh/storage/tests/test_api_client_dicts.py |  51 ----
 swh/storage/tests/test_replay.py           | 438 ++++++++---------------------
 swh/storage/tests/test_storage.py          |  17 +-
 swh/storage/tests/test_write_replay.py     | 112 --------
 swh/storage/validate.py                    |  25 +-
 10 files changed, 175 insertions(+), 508 deletions(-)
 delete mode 100644 swh/storage/tests/test_api_client_dicts.py
 delete mode 100644 swh/storage/tests/test_write_replay.py

Changes applied before test

commit f9b2ca3fec14853281ca7236741398e09b90e88e
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri May 29 14:28:31 2020 +0200

    Replace MockedJournalClient and MockedKafkaWriter by proper kafka test scaffolding
    
    This also kills test_write_replay.py file since it does not test anything
    more than what is currently tested in test_replay.py.

commit ad9c9bbfd0d536217e895fd41408c8c11ceafc66
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Jun 2 17:25:18 2020 +0200

    Adapt to swh.model 0.3
    
    in which List attributes have replaced by Tuple ones.
    
    This requires a bit of adaptation in the code of the
    ValidatingProxyStorage to ensure dict representation of revision
    objects are properly typed.
    
    The test_api_client_dicts.py has been removed since it's not really
    useful any more and would require a fair amount of work to fix it.

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

Harbormaster completed remote builds in B12624: Diff 11420.Jun 4 2020, 12:01 PM

Closed by commit rDSTOf9b2ca3fec14: Replace MockedJournalClient and MockedKafkaWriter by proper kafka test… (authored by douardda). · Explain WhyJun 4 2020, 12:03 PM

This revision was automatically updated to reflect the committed changes.

douardda added a commit: rDSTOf9b2ca3fec14: Replace MockedJournalClient and MockedKafkaWriter by proper kafka test….