HEPCloud / decisionengine

HEPCloud Decision Engine framework
Apache License 2.0
6 stars 25 forks source link

Isolate Redis DBs during unit tests #624

Closed knoepfel closed 2 years ago

knoepfel commented 2 years ago

The DE server configuration for unit tests uses an exchange with the name hep_topic_exchange, and it uses the same Redis URL for each test. This is a problem whenever independent unit tests use the same routing keys, resulting in Kombu message collisions.

This PR attempts to mitigate this problem by assigning different Redis databases per DE server fixture. If pytest-xdist is setup, the xdist worker number will be used as the database number. Otherwise, a number between 0 and 15 (inclusive) will be randomly picked as the database number--Redis supports 16 databases by default.

N.B. This PR is based off of PRs #620 and #623.

codecov[bot] commented 2 years ago

Codecov Report

Merging #624 (c8f3c04) into master (e36c215) will increase coverage by 0.61%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #624      +/-   ##
==========================================
+ Coverage   96.73%   97.35%   +0.61%     
==========================================
  Files          47       47              
  Lines        2822     2838      +16     
  Branches      379      449      +70     
==========================================
+ Hits         2730     2763      +33     
+ Misses         59       50       -9     
+ Partials       33       25       -8     
Flag Coverage Δ
python-3.10 97.11% <100.00%> (?)
python-3.6 96.84% <100.00%> (+0.10%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../decisionengine/framework/engine/DecisionEngine.py 92.02% <100.00%> (+0.48%) :arrow_up:
src/decisionengine/framework/modules/Transform.py 100.00% <0.00%> (ø)
...ionengine/framework/taskmanager/ProcessingState.py 100.00% <0.00%> (ø)
...rc/decisionengine/framework/dataspace/dataspace.py 93.97% <0.00%> (+0.07%) :arrow_up:
...aspace/datasources/sqlalchemy_ds/datasource_api.py 100.00% <0.00%> (+1.22%) :arrow_up:
.../decisionengine/framework/engine/ChannelWorkers.py 90.78% <0.00%> (+2.63%) :arrow_up:
src/decisionengine/framework/dataspace/maintain.py 100.00% <0.00%> (+3.96%) :arrow_up:
...sionengine/framework/taskmanager/LatestMessages.py 100.00% <0.00%> (+4.44%) :arrow_up:
...ework/dataspace/datasources/sqlalchemy_ds/utils.py 100.00% <0.00%> (+15.62%) :arrow_up:

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 e36c215...c8f3c04. Read the comment docs.

knoepfel commented 2 years ago

@BrunoCoimbra, what do you mean by "starting from the 17th"?

BrunoCoimbra commented 2 years ago

Starting from the 17th worker. Since Redis only supports 16 databases by default, whenever we have more than 16 workers, some of them would share databases, right?

knoepfel commented 2 years ago

Ah. If we ever run tests with 17 pytest-xdist workers (i.e. 17 pytest unit tests running concurrently), then yes, at least one Redis database will be shared. Right now, we use 4 xdist workers, so there's no potential of collision.

For production purposes, only one DE server is active per node, so there won't be any collisions there either.