Skip to content

Conversation

@mnoman09
Copy link
Contributor

Summary

  • IsExperiment running check is added in rollout evaluation to make sure paused rollout experiment gets skipped.

Test plan

All FSC and unit tests should pass.

@mnoman09 mnoman09 requested a review from a team as a code owner November 22, 2021 15:57
@coveralls
Copy link

coveralls commented Nov 22, 2021

Pull Request Test Coverage Report for Build 1921

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 90.757%

Totals Coverage Status
Change from base Build 1914: 0.02%
Covered Lines: 4870
Relevant Lines: 5366

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1903

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 90.738%

Totals Coverage Status
Change from base Build 1898: 0.02%
Covered Lines: 4879
Relevant Lines: 5377

💛 - Coveralls

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

int index = 0;
while (index < rolloutRulesLength) {

if (!rollout.getExperiments().get(index).isRunning()) {
Copy link
Contributor

@The-inside-man The-inside-man Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check the status of the experiment to be "paused" rather than not isRunning since there are 4 other status'? Or is this behavior supposed to be for every status other than running?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants