fix intermittent test coverage #2891

New issue

Open

opened 2017-07-23 16:58:50 +00:00 by warner · 8 comments

warner commented

2017-07-23 16:58:50 +00:00

The most annoying thing about these tests is when they cause perfectly valid pull requests to be flagged as broken (because the test coverage happened to drop when they went through CI, through no fault of the PR's changes).

Let's ~~strikeout~~ these items as we fix them.

src/allmydata/mutable/servermap.py
the status object's .timings["per_server"] attribute is a dict-of-lists, and line 39 handles the case where we don't have to add a new entry to the dict. This should just be replaced by a collections.defaultdict, rather than needing any new tests.
[L 923: ServermapUpdater._try_to_validate_privkey](74e7ef4b98/src/allmydata/mutable/servermap.py (L923))
this clause checks that the share contained an (encrypted) RSA signing key that matches the expectation we got from the writecap. The clause should succeed for normal shares, but will fail for corrupted shares. Apparently we have a test that corrupts some shares, but which then doesn't always look at them.
src/allmydata/mutable/publish.py
L 451: Publish.publish
to exercise this, we need some bad shares recorded in the servermap (ticket:3540)
L 496 Publish.publish
src/allmydata/immutable/upload.py
Tahoe2ServerSelector._buckets_allocated()
the if s in self.homeless_shares inside the non-Failure path doesn't always exercise both sides of the branch
src/allmydata/immutable/downloader/share.py
Share.loop()
this share-is-corrupted branch is not always exercised
L228-236
the DataUnavailable branch is not always exercised
L277-279 Share._do_loop()
this doesn't always see a non-empty disappointment array, and sometimes doesn't raise DataUnavailable. This is the parent cause of the L228-L236 non-coverage above.
L386-389 _satisfy_offsets()
the share_hashes_size test doesn't exercise both sides of the branch
L761 _got_data()
the if not self._alive check doesn't always exercise both branches
src/allmydata/util/dictutil.py
src/allmydata/stats.py
[L64-68 LoadMonitor.get_stats()](3f2f7dfb05/src/allmydata/stats.py (L68))
noticed in a build of PR421
get_stats() returns an average of the last 60 datapoints, but if there are no datapoints at all, a different branch is taken
that no-datapoints branch is sometimes not covered by the tests
src/allmydata/web/status.py
also from that PR421 build
[L758 RetrieveStatusPage.render_server_timings()](3f2f7dfb05/src/allmydata/web/status.py (L758))
sometimes the fetch_per_server timing data is empty, which skips parts of this function
[L925 MapupdateStatusPage.render_privkey_from()](3f2f7dfb05/src/allmydata/web/status.py (L925))
sometimes this is called when the privkey has not been fetched, so it skips the affirmative side of the branch
L1117 Status.childFactory()
the loop that looks for a "down-%s" name in h.list_all_download_statuses() doesn't always exercise both sides of the if s.get_counter() == count clause

This ticket is to keep track of lines which sometimes get covered during unit tests and which sometimes do not. This probably happens because those lines are only exercised by "system" -style tests, which randomly hit some servers and not others. We should fix these by writing more narrowly-focussed "unit"-style tests that exercise just the function in question, deterministically. The most annoying thing about these tests is when they cause perfectly valid pull requests to be flagged as broken (because the test coverage happened to drop when they went through CI, through no fault of the PR's changes). Let's ~~strikeout~~ these items as we fix them. * src/allmydata/mutable/servermap.py * ~~~[L 39: [UpdateStatus](wiki/UpdateStatus).add_per_server_time](https://github.com/tahoe-lafs/tahoe-lafs/blob/74e7ef4b98bc322da5a49c33e0706b57aab3a6d3/src/allmydata/mutable/servermap.py#L39)~~~ * the status object's `.timings["per_server"]` attribute is a dict-of-lists, and line 39 handles the case where we *don't* have to add a new entry to the dict. This should just be replaced by a `collections.defaultdict`, rather than needing any new tests. * [L 923: [ServermapUpdater](wiki/ServermapUpdater)._try_to_validate_privkey](https://github.com/tahoe-lafs/tahoe-lafs/blob/74e7ef4b98bc322da5a49c33e0706b57aab3a6d3/src/allmydata/mutable/servermap.py#L923) * this clause checks that the share contained an (encrypted) RSA signing key that matches the expectation we got from the writecap. The clause should succeed for normal shares, but will fail for corrupted shares. Apparently we have a test that corrupts some shares, but which then doesn't always look at them. * src/allmydata/mutable/publish.py * [L 451: Publish.publish](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/mutable/publish.py#L451) * to exercise this, we need some bad shares recorded in the servermap (ticket:3540) * [L 496 Publish.publish](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/mutable/publish.py#L496) * src/allmydata/immutable/upload.py * [Tahoe2ServerSelector._buckets_allocated()](https://github.com/tahoe-lafs/tahoe-lafs/blob/d91516a5c5c7d6c6a8d040aed4302aa2b9e3bf90/src/allmydata/immutable/upload.py#L703) * the `if s in self.homeless_shares` inside the non-Failure path doesn't always exercise both sides of the branch * src/allmydata/immutable/downloader/share.py * [Share.loop()](https://github.com/tahoe-lafs/tahoe-lafs/blob/d91516a5c5c7d6c6a8d040aed4302aa2b9e3bf90/src/allmydata/immutable/downloader/share.py#L211) * this share-is-corrupted branch is not always exercised * L228-236 * the DataUnavailable branch is not always exercised * [L277-279 Share._do_loop()](https://github.com/tahoe-lafs/tahoe-lafs/blob/d91516a5c5c7d6c6a8d040aed4302aa2b9e3bf90/src/allmydata/immutable/downloader/share.py#L277) * this doesn't always see a non-empty `disappointment` array, and sometimes doesn't raise DataUnavailable. This is the parent cause of the L228-L236 non-coverage above. * [L386-389 _satisfy_offsets()](https://github.com/tahoe-lafs/tahoe-lafs/blob/d91516a5c5c7d6c6a8d040aed4302aa2b9e3bf90/src/allmydata/immutable/downloader/share.py#L386) * the share_hashes_size test doesn't exercise both sides of the branch * [L761 _got_data()](https://github.com/tahoe-lafs/tahoe-lafs/blob/d91516a5c5c7d6c6a8d040aed4302aa2b9e3bf90/src/allmydata/immutable/downloader/share.py#L386) * the `if not self._alive` check doesn't always exercise both branches * src/allmydata/util/dictutil.py * ~~~[L 29 subtract()](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/util/dictutil.py#L29)~~~ * ~~~[L 230 [NumDict](wiki/NumDict).item_with_largest_value()](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/util/dictutil.py#L230)~~~ * ~~~[L 438 [ValueOrderedDict](wiki/ValueOrderedDict).*repr_n*](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/util/dictutil.py#L438)~~~ * ~~~[L 507 [ValueOrderedDict](wiki/ValueOrderedDict).*setitem*](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/util/dictutil.py#L507)~~~ * ~~~[L 511 [ValueOrderedDict](wiki/ValueOrderedDict).*setitem*](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/util/dictutil.py#L511)~~~ * ~~~[L 550 [ValueOrderedDict](wiki/ValueOrderedDict).*delitem*](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/util/dictutil.py#L550)~~~ * ~~~[L 597 [ValueOrderedDict](wiki/ValueOrderedDict).pop](https://github.com/tahoe-lafs/tahoe-lafs/blob/95ac5494ffaad61494e0616286894a20b87b7da8/src/allmydata/util/dictutil.py#L597)~~~ * src/allmydata/stats.py * [L64-68 [LoadMonitor](wiki/LoadMonitor).get_stats()](https://github.com/tahoe-lafs/tahoe-lafs/blob/3f2f7dfb05ad7f505b70323459aec8d70d2a3ab5/src/allmydata/stats.py#L68) * noticed in a [build of PR421](https://codecov.io/gh/tahoe-lafs/tahoe-lafs/pull/421/changes) * get_stats() returns an average of the last 60 datapoints, but if there are no datapoints at all, a different branch is taken * that no-datapoints branch is sometimes not covered by the tests * src/allmydata/web/status.py * also from that PR421 build * [L758 [RetrieveStatusPage](wiki/RetrieveStatusPage).render_server_timings()](https://github.com/tahoe-lafs/tahoe-lafs/blob/3f2f7dfb05ad7f505b70323459aec8d70d2a3ab5/src/allmydata/web/status.py#L758) * sometimes the `fetch_per_server` timing data is empty, which skips parts of this function * [L925 [MapupdateStatusPage](wiki/MapupdateStatusPage).render_privkey_from()](https://github.com/tahoe-lafs/tahoe-lafs/blob/3f2f7dfb05ad7f505b70323459aec8d70d2a3ab5/src/allmydata/web/status.py#L925) * sometimes this is called when the privkey has not been fetched, so it skips the affirmative side of the branch * [L1117 Status.childFactory()](https://github.com/tahoe-lafs/tahoe-lafs/blob/d91516a5c5c7d6c6a8d040aed4302aa2b9e3bf90/src/allmydata/web/status.py#L1117) * the loop that looks for a "down-%s" name in h.list_all_download_statuses() doesn't always exercise both sides of the `if s.get_counter() == count` clause

warner added the

labels 2017-07-23 16:58:50 +00:00

warner added this to the undecided milestone 2017-07-23 16:58:50 +00:00

Brian Warner <warner@lothar.com> commented

2017-07-28 04:04:30 +00:00

Owner

In a4be2dc/trunk:

avoid variable coverage by using a defaultdict

refs ticket:2891

In [a4be2dc/trunk](/tahoe-lafs/trac/commit/a4be2dce71ef913037cc27c12c29f96dc88da9db): ``` avoid variable coverage by using a defaultdict refs ticket:2891 ```

warner commented

2017-08-10 17:40:23 +00:00

Author

added dictutil misses, updated format of the table, struck out the add_per_server_time that was fixed in a4be2dc

Brian Warner <warner@lothar.com> commented

2017-08-10 18:40:06 +00:00

Owner

In 3f2f7df/trunk:

dictutil: fix bug in str(ValueOrderedDict), and improve test coverage

It looks like str() was meant to truncate the dict, but a missing i+=1 meant
that it never actually did. I also changed the format to include a clear
"..." in case we truncate it, to avoid confusion with a non-truncated dict of
the same size.

This also improves test coverage in subtract() and
NumDict.item_with_largest_value().

refs ticket:2891

In [3f2f7df/trunk](/tahoe-lafs/trac/commit/3f2f7dfb05ad7f505b70323459aec8d70d2a3ab5): ``` dictutil: fix bug in str(ValueOrderedDict), and improve test coverage It looks like str() was meant to truncate the dict, but a missing i+=1 meant that it never actually did. I also changed the format to include a clear "..." in case we truncate it, to avoid confusion with a non-truncated dict of the same size. This also improves test coverage in subtract() and NumDict.item_with_largest_value(). refs ticket:2891 ```

warner commented

2017-08-10 18:41:50 +00:00

Author

fixed dictutil.py (lines 29, 230, 438) with [3f2f7df]

warner commented

2017-08-10 20:02:55 +00:00

Author

added intermittence from an old build of PR421 (stats.py, web/status.py)

warner commented

2017-08-10 21:59:23 +00:00

Author

more intermittence noticed in tests: immutable/upload.py and immutable/downloader/share.py

Brian Warner <warner@lothar.com> commented

2017-08-15 21:12:15 +00:00

Owner

In 27348be/trunk:

Merge PR438 from branch '2891-remove-numdict'

This removes some code in dictutil.py that we weren't using, or which could
be replaced by something simpler. This code is troublesome, because our unit
tests only achieve intermittent coverage, so other (unrelated) PRs are
failing CI when the coverage appears to go down.

I tried to improve the tests to reliably cover everything in dictutil.py, and
discovered code that couldn't possibly have worked in the first place. So the
easiest approach was just to delete it all.

refs ticket:2891

In [27348be/trunk](/tahoe-lafs/trac/commit/27348be795d163d79d0cdefae8cb024d26fd81db): ``` Merge PR438 from branch '2891-remove-numdict' This removes some code in dictutil.py that we weren't using, or which could be replaced by something simpler. This code is troublesome, because our unit tests only achieve intermittent coverage, so other (unrelated) PRs are failing CI when the coverage appears to go down. I tried to improve the tests to reliably cover everything in dictutil.py, and discovered code that couldn't possibly have worked in the first place. So the easiest approach was just to delete it all. refs ticket:2891 ```