Cover can_bundle.py in integration tests (#3196) #3237

nadove-ucsc · 2021-07-15T06:03:55Z

PR title references issue
Title of main commit references issue
PR is linked to Zenhub issue

Author (reindex)

Added r tag to commit title _{or this PR does not require reindexing}
Added reindex label to PR _{or this PR does not require reindexing}

Author (freebies & chains)

Freebies are blocked on this PR _{or there are no freebies in this PR}
Freebies are referenced in commit titles _{or there are no freebies in this PR}
This PR is blocked by previous PR in the chain _{or this PR is not chained to another PR}
Added chain label to the blocking PR _{or this PR is not chained to another PR}

Author (upgrading)

Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading}
Added u tag to commit title _{or this PR does not require upgrading}
Added upgrade label to PR _{or this PR does not require upgrading}

Author (requirements, before every review)

Ran make requirements_update _{or this PR leaves requirements*.txt, common.mk and Makefile untouched}
Added R tag to commit title _{or this PR leaves requirements*.txt untouched}
Added reqs label to PR _{or this PR leaves requirements*.txt untouched}

Author (before every review)

make integration_test passes in personal deployment _{or this PR does not touch functionality that could break the IT}
Rebased branch on develop, squashed old fixups

Primary reviewer (after approval)

Commented in issue about demo expectations _{or labelled issue as no demo}
Decided if PR can be labeled no sandbox
PR title is appropriate as title of merge commit
Moved ticket to Approved column
Assigned PR to an operator

Operator (before pushing merge the commit)

Operator (after pushing the merge commit)

Moved freebies to Merged column _{or there are no freebies in this PR}
Shortened the PR chain _{or this PR is not the base of another PR}
Verified that N reviews labelling is accurate
Pushed merge commit to Gitlab _{or this changes can be pushed later, together with another PR}
Deleted PR branch from Github and Gitlab

Operator (reindex)

Started reindex in dev _{or this PR does not require reindexing or does not target dev}
Checked for failures in dev _{or this PR does not require reindexing or does not target dev}
Started reindex in prod _{or this PR does not require reindexing or does not target prod}
Checked for failures in prod _{or this PR does not require reindexing or does not target prod}

Operator

Unassigned PR

codecov · 2021-07-15T06:19:57Z

Codecov Report

Merging #3237 (22175e3) into develop (eba9a93) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 22175e3 differs from pull request most recent head b764fed. Consider uploading reports for the commit b764fed to get more accurate results

@@             Coverage Diff             @@
##           develop    #3237      +/-   ##
===========================================
+ Coverage    82.55%   82.58%   +0.02%     
===========================================
  Files          123      123              
  Lines        14246    14254       +8     
===========================================
+ Hits         11761    11771      +10     
+ Misses        2485     2483       -2

Impacted Files	Coverage Δ
src/azul/__init__.py	`81.48% <100.00%> (+0.11%)`	⬆️
src/azul/service/elasticsearch_service.py	`83.22% <100.00%> (+0.52%)`	⬆️
src/azul/service/hca_response_v5.py	`92.41% <100.00%> (ø)`
src/azul/service/index_query_service.py	`90.19% <100.00%> (+0.50%)`	⬆️
src/azul/terra.py	`63.71% <0.00%> (-0.19%)`	⬇️
test/service/test_response.py	`99.84% <0.00%> (ø)`
src/azul/bigquery_reservation.py	`0.00% <0.00%> (ø)`
test/indexer/test_hca_indexer.py	`99.30% <0.00%> (ø)`
src/azul/plugins/metadata/hca/__init__.py	`100.00% <0.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eba9a93...b764fed. Read the comment docs.

coveralls · 2021-07-15T06:20:45Z

Coverage decreased (-0.3%) to 82.282% when pulling 07397b0 on issues/noah-aviel-dove/3196-it-can-bundle into 051ac80 on develop.

jessebrennan · 2021-07-15T16:15:52Z

test/integration_test.py

+            '--output-dir', output_dir
+        ]
+        args = [shlex.quote(arg) for arg in args]
+        result = subprocess.run(args)


Why not just import and call main() directly? It would make debugging easier in the case of a failure.

I thought we weren't supposed to import from scripts/ from other locations

Now that you mention it, I remember hearing that too... though I wonder if testing a script like this is worthwhile exception.

I think we have to make it an exception because mock.patch can't cross process boundaries, and we need to patch the configured catalogs in order to test the canned repository.

(Worth noting that the Pycharm debugger does seem to be capable of crossing process boundaries. When I put a breakpoint in the script and invoked it via subprocess.run, it stopped at the breakpoint).

(Worth noting that the Pycharm debugger does seem to be capable of crossing process boundaries. When I put a breakpoint in the script and invoked it via subprocess.run, it stopped at the breakpoint).

Good to know, thanks for testing this.

That can be turned on or off.

Note load_script in azul.modules.

jessebrennan · 2021-07-19T16:29:20Z

test/integration_test.py

+        plugin = RepositoryPlugin.load(catalog).create(catalog)
+        spec = first(plugin.sources)
+        source = plugin.resolve_source(str(spec))
+        return first(plugin.list_bundles(source, prefix=source.spec.prefix))


This probably isn't worth troubling about, but the list_bundles query fetches all the bundles with that prefix when you're only trying to get one. Ideally the size could be specified in the query.

list_bundles query fetches all the bundles with that prefix

This might be just a confusion of terminology, but RepositoryPlugin.list_bundles does not fetch any bundles, it only lists the bundle FQIDs. RepositoryPlugin.fetch_bundle is a separate method.

It still lists all bundles even though it only needs one. This will definitely cause trouble in dev and prod where no prefix is used on the IT catalogs and 100k+ bundles will be returned. The indexing IT uses a technique to shrink a randomly chosen prefix until 64 bundles match. You should use that or something like that here too. Second, it would be nice to use a randomly selected bundle but the indexing test prefix selection already takes care of that. As always, it should be possible to seed the random generator with minimal code changes in order to be able to reproduce IT failures.

test/integration_test.py

jessebrennan

I approve, but I think something is wrong with the history. The first commit shouldn't show up in this PR, but rather in the PR for 3249. Probably you just need to rebase onto that branch again.

hannes-ucsc · 2021-07-21T23:07:18Z

Should I be assigned?

github-actions bot added the orange [process] Done by the Azul team label Jul 15, 2021

nadove-ucsc changed the title ~~Add coverage for can_bundle.py (#3196)~~ Cover can_bundle.py in integration tests (#3196) Jul 15, 2021

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch 3 times, most recently from 31d1b85 to ff05567 Compare July 15, 2021 06:58

jessebrennan reviewed Jul 15, 2021

View reviewed changes

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from ff05567 to 06b870f Compare July 16, 2021 00:45

nadove-ucsc requested a review from jessebrennan July 16, 2021 01:16

nadove-ucsc assigned jessebrennan Jul 16, 2021

jessebrennan suggested changes Jul 19, 2021

View reviewed changes

jessebrennan removed their assignment Jul 19, 2021

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch 2 times, most recently from 7332d6c to 835f65c Compare July 20, 2021 19:37

nadove-ucsc changed the base branch from develop to issues/noah-aviel-dove/3249-move-scripts-args July 20, 2021 19:39

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch 3 times, most recently from 87a3a24 to 6d9f439 Compare July 20, 2021 19:55

nadove-ucsc force-pushed the issues/noah-aviel-dove/3249-move-scripts-args branch from 6660151 to 591ae73 Compare July 21, 2021 01:28

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from 6d9f439 to 4c9c7f6 Compare July 21, 2021 01:48

nadove-ucsc requested a review from jessebrennan July 21, 2021 02:12

nadove-ucsc assigned jessebrennan Jul 21, 2021

jessebrennan approved these changes Jul 21, 2021

View reviewed changes

jessebrennan removed their assignment Jul 21, 2021

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch 2 times, most recently from 6a4091a to 57405b0 Compare July 21, 2021 20:01

nadove-ucsc requested a review from hannes-ucsc July 21, 2021 20:37

nadove-ucsc assigned hannes-ucsc Jul 21, 2021

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from caee57c to ae262b5 Compare September 15, 2021 10:54

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch 2 times, most recently from ddea6a5 to 717e8b6 Compare September 23, 2021 02:09

nadove-ucsc changed the base branch from develop to issues/noah-aviel-dove/3452-it-nondeterminism September 23, 2021 02:10

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from 717e8b6 to f9704f2 Compare September 23, 2021 02:29

nadove-ucsc force-pushed the issues/noah-aviel-dove/3452-it-nondeterminism branch from 3d8cee4 to 9eebd30 Compare September 23, 2021 22:50

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from f9704f2 to b5390c0 Compare September 23, 2021 23:18

nadove-ucsc force-pushed the issues/noah-aviel-dove/3452-it-nondeterminism branch 2 times, most recently from 3977d35 to cfaaacb Compare September 28, 2021 01:36

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from b5390c0 to c92e549 Compare September 28, 2021 03:29

nadove-ucsc changed the base branch from issues/noah-aviel-dove/3452-it-nondeterminism to develop September 28, 2021 03:29

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from c92e549 to 68ac757 Compare September 28, 2021 08:35

hannes-ucsc force-pushed the develop branch from 22175e3 to da4a172 Compare September 28, 2021 18:02

nadove-ucsc removed the sandbox [process] Resolution is being verified in sandbox deployment label Sep 28, 2021

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from 68ac757 to b764fed Compare September 29, 2021 02:49

nadove-ucsc requested a review from hannes-ucsc September 29, 2021 05:38

nadove-ucsc assigned hannes-ucsc Sep 29, 2021

hannes-ucsc approved these changes Sep 29, 2021

View reviewed changes

hannes-ucsc removed the hold warm [process] PR can't land just yet but needs to be rebased daily label Sep 29, 2021

hannes-ucsc assigned nadove-ucsc and unassigned hannes-ucsc Sep 29, 2021

nadove-ucsc and others added 4 commits September 30, 2021 21:23

Fix typo

3377882

Add ability to temporarily stash cached properties

7b6412c

Refactor bundle pruning in integration test

9127c8a

[u] Cover can_bundle.py in integration tests (#3196)

07397b0

nadove-ucsc force-pushed the issues/noah-aviel-dove/3196-it-can-bundle branch from b764fed to 07397b0 Compare October 1, 2021 04:23

nadove-ucsc merged commit 2fbf613 into develop Oct 1, 2021

nadove-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Oct 1, 2021

nadove-ucsc deleted the issues/noah-aviel-dove/3196-it-can-bundle branch October 1, 2021 05:51

nadove-ucsc removed their assignment Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cover can_bundle.py in integration tests (#3196) #3237

Cover can_bundle.py in integration tests (#3196) #3237

nadove-ucsc commented Jul 15, 2021 •

edited

Loading

codecov bot commented Jul 15, 2021 •

edited

Loading

coveralls commented Jul 15, 2021 •

edited

Loading

jessebrennan Jul 15, 2021

nadove-ucsc Jul 15, 2021

jessebrennan Jul 15, 2021

nadove-ucsc Jul 15, 2021

nadove-ucsc Jul 15, 2021

jessebrennan Jul 15, 2021

hannes-ucsc Jul 16, 2021 •

edited

Loading

jessebrennan Jul 19, 2021

nadove-ucsc Jul 19, 2021

hannes-ucsc Jul 22, 2021

jessebrennan left a comment

hannes-ucsc commented Jul 21, 2021

Cover can_bundle.py in integration tests (#3196) #3237

Cover can_bundle.py in integration tests (#3196) #3237

Conversation

nadove-ucsc commented Jul 15, 2021 • edited Loading

codecov bot commented Jul 15, 2021 • edited Loading

Codecov Report

coveralls commented Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannes-ucsc Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessebrennan left a comment

Choose a reason for hiding this comment

hannes-ucsc commented Jul 21, 2021

nadove-ucsc commented Jul 15, 2021 •

edited

Loading

codecov bot commented Jul 15, 2021 •

edited

Loading

coveralls commented Jul 15, 2021 •

edited

Loading

hannes-ucsc Jul 16, 2021 •

edited

Loading