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.

Sign up for GitHub

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

Jump to bottom

[tests-only] retry if auto-sync is enabled but client.synchronize is false #8891

Closed
wants to merge 3 commits into from
Closed

[tests-only] retry if auto-sync is enabled but client.synchronize is false #8891

wants to merge 3 commits into from

Conversation

saw-jan
Copy link
Member

@saw-jan saw-jan commented Apr 18, 2024 •
edited

Description

Implemented retry when listing the shares if auto-sync is enabled but client.synchronize is false. If the auto-sync is disabled then the retry will be skipped.

Scenarios can pass with retry:

    When user "Brian" lists the shares shared with him using the Graph API
      │ auto-sync share for user 'Brian' is enabled
      │ but share 'textfile0.txt' was not auto-synced, retrying (1)...
      │ 
    Then the HTTP status code should be "200"

Related Issue

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@saw-jan saw-jan self-assigned this Apr 18, 2024
@saw-jan saw-jan force-pushed the tests/flaky-sharing-tests branch 10 times, most recently from 5e3378e to 939c605 Compare April 25, 2024 05:12
@saw-jan saw-jan changed the title [do-not-merge][tests-only] check flaky sharing test [do-not-merge][tests-only] retry if auto-sync is enabled but client.synchronize is false Apr 25, 2024
@saw-jan saw-jan changed the title [do-not-merge][tests-only] retry if auto-sync is enabled but client.synchronize is false [tests-only] retry if auto-sync is enabled but client.synchronize is false Apr 25, 2024
@saw-jan saw-jan marked this pull request as ready for review April 25, 2024 07:06
@saw-jan saw-jan marked this pull request as draft April 25, 2024 07:11
@saw-jan
Copy link
Member Author

saw-jan commented Apr 25, 2024 •
edited

Want to check the edge cases Ready for review

@saw-jan saw-jan marked this pull request as ready for review April 25, 2024 09:28
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Apr 29, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

amrita-shrestha
amrita-shrestha reviewed Apr 30, 2024
View reviewed changes
tests/acceptance/features/bootstrap/GraphContext.php
);
);

$jsonObj = $this->featureContext->getJsonDecodedResponseBodyContent($response);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$jsonObj = $this->featureContext->getJsonDecodedResponseBodyContent($response);
$jsonObj = $this->featureContext->getJsonDecodedResponseBodyContent($response);

$jsonObj name should be replace with meaningful name

amrita-shrestha
amrita-shrestha reviewed Apr 30, 2024
View reviewed changes
tests/acceptance/features/bootstrap/GraphContext.php
break;
}
}
} while ($tryAgain);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} while ($tryAgain);
} while ($tryAgain);

isn't it dangerous to use. I think we should limit tryAgain ?

Copy link
Member Author

Choose a reason for hiding this comment

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

this line should prevent the infinite loop

$tryAgain = !$share->{'@client.synchronize'} && $autoSync && $retried < HttpRequestHelper::numRetriesOnHttpTooEarly();

@saw-jan
Copy link
Member Author

saw-jan commented May 6, 2024 •
edited

Previously flaky scenarios are consistent with this explicit wait 4e66cc7. Though it is not so good to have hard coded wait, the fix in this PR (retry step) is also not that much promising because it can lead to confusion in the future where there might be scenarios that manipulate share-sync config in different ways.

So, let's keep the wait fix and I will close this PR for now.
If anyone has any thought then please let me know.

CC @ScharfViktor

@saw-jan saw-jan closed this May 6, 2024
@saw-jan saw-jan deleted the tests/flaky-sharing-tests branch May 6, 2024 06:34
@saw-jan saw-jan mentioned this pull request May 6, 2024
Open
@ScharfViktor
Copy link
Contributor

to avoid flaky we can check that sychronize is true using step with timeout lists the shares shared with him after clearing user cache using the Graph API

for another scenarios we can check it like true,false:

"@client.synchronize": {
                "type": "boolean",
                "enum": [true,false]
              },

@saw-jan
Copy link
Member Author

saw-jan commented May 6, 2024

Let's keep the current fix. We can come back to this if we get other flakiness

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

@amrita-shrestha amrita-shrestha amrita-shrestha left review comments

@grgprarup grgprarup Awaiting requested review from grgprarup

@SagarGi SagarGi Awaiting requested review from SagarGi

@S-Panta S-Panta Awaiting requested review from S-Panta

@PrajwolAmatya PrajwolAmatya Awaiting requested review from PrajwolAmatya

@ScharfViktor ScharfViktor Awaiting requested review from ScharfViktor

@nirajacharya2 nirajacharya2 Awaiting requested review from nirajacharya2

Assignees

@saw-jan saw-jan

Labels
QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auto-accepting is slow in CI [QA] Flaky API test
3 participants
@saw-jan @ScharfViktor @amrita-shrestha