Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix test_wait_for_handler_completion_no_status test #2548

Merged
merged 2 commits into from
Apr 18, 2022

Conversation

nagworld9
Copy link
Contributor

Description

Seems this UT has timing issue. Our current mocked timeout is too soon and not able to capture the status which we are testing in UT. Also, we have used different timeout for the same variable in other test. So I'm increasing to that number which could fix the problem.

Issue #


PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made

Quality of Code and Contribution Guidelines

@codecov
Copy link

codecov bot commented Apr 13, 2022

Codecov Report

Merging #2548 (2e34d19) into develop (ae5cff8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2548   +/-   ##
========================================
  Coverage    71.76%   71.76%           
========================================
  Files          102      102           
  Lines        15314    15314           
  Branches      2426     2426           
========================================
  Hits         10990    10990           
  Misses        3834     3834           
  Partials       490      490           

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 ae5cff8...2e34d19. Read the comment docs.

@@ -1453,7 +1453,7 @@ def mock_popen(cmd, *args, **kwargs):
return original_popen(["echo", "Yes"], *args, **kwargs)

with patch('azurelinuxagent.common.cgroupapi.subprocess.Popen', side_effect=mock_popen):
with patch('azurelinuxagent.ga.exthandlers._DEFAULT_EXT_TIMEOUT_MINUTES', 0.001):
with patch('azurelinuxagent.ga.exthandlers._DEFAULT_EXT_TIMEOUT_MINUTES', 0.01):
Copy link
Contributor Author

@nagworld9 nagworld9 Apr 13, 2022

Choose a reason for hiding this comment

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

 ext_completed, status = False, None

            # Keep polling for the extension status until it succeeds or times out
            while datetime.datetime.utcnow() <= wait_until:
                ext_completed, status = handler_i.is_ext_handling_complete(extension)
                if ext_completed:
                    break
                time.sleep(5)

Status should be retrieved from handler_i.is_ext_handling_complete but while didn't run because timeout could have happened before. So, status had defaults.

Copy link
Member

Choose a reason for hiding this comment

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

but the test extension is not async, right? doesn't it completely sychronously?

Copy link
Member

Choose a reason for hiding this comment

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

but the test extension is not async, right? doesn't it completely sychronously?

Copy link
Contributor Author

@nagworld9 nagworld9 Apr 13, 2022

Choose a reason for hiding this comment

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

Maybe I didn't explain you properly. The UT is to test base extension status "transitioning" if status file not found and status of dependent extension. The timeout I mentioned is how long we wait keep polling and report status. If it's still not completed within timeout, then we report last status with timeout issue on dependent extension handler status. In this case sometimes the polling won't happen at least once because wait period already completed by the time status code path executed and extension processing might take little longer or crossed the timeout mins we mocked in UT.

but the test extension is not async, right? doesn't it completely sychronously?
yes, it runs synchronously. The issue is with in extension code path execution and not with UT vs extension time. when we start the extension, we say extension processing needs to complete in 'x' seconds and wait and keeps polling the status until 'x' seconds. But in this case status polling not happened because series of steps before this might took more than 'x' seconds. That's why this condition while datetime.datetime.utcnow() <= wait_until failed and polling didn't happen. I hope this answers your question.

@narrieta narrieta merged commit 02db57b into Azure:develop Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants