Improve Gitlab::Database::Partitioning tests
Improve Gitlab::Database::Partitioning
tests not to call private methods (using __send__
):
- setup state using
Gitlab::Database::Partitioning.registered_models
- move the responsibility to ensure that the registered models have partitioning strategy to
register_models
. - and make sure we do not leak state outside tests
This brought up during the review of !72697 (merged):
-
@furkanayhan started a discussion: (+2 comments) I'm confused with this?
🤔 We assigndescribed_class.registered_for_sync
to a variable, then stub the same method to the same variable, then use the same variable😅 I assume it's something to have the same return value from
registered_models
, but... should we do this?🤔 -
@krasio started a discussion: Similar to above, we can have this test like
it 'reports metrics for each registered model' do expect_next_instance_of(partition_monitoring_class) do |partition_monitor| expect(partition_monitor).to receive(:report_metrics_for_model).with(models.first) expect(partition_monitor).to receive(:report_metrics_for_model).with(models.last) end described_class.instance_variable_set('@registered_models', Set.new) described_class.register_models(models) described_class.report_metrics end end
though having to use
instance_variable_set
is not ideal, and means we miss method likereset
orregistered_models=
. -
@krasio started a discussion: Does having to use
__send__
mean we need to keepregistered_models
to public?BTW I think "ensure that the registered models have partitioning strategy" should be responsibility of
register_models
/register_tables
- we should check and raise if the model provided does not respond topartitioning_strategy
.