Skip to content

Conversation

@sadasant
Copy link
Contributor

@sadasant sadasant commented Apr 28, 2021

This PR is now about skipping device code tests on playback. Live tests are staying for now, since we want to use them to release. I'll be working on a fix on MSAL's repo.

We're aware of a CI issue in which something gets stuck during tests, meaning that the Node process never closes, which timeouts the core pipeline. We are not 100% sure on what's the underlying issue, because we haven't been able to reproduce the problem locally.

However, we're aware of one logical case in which this may happen. There's an issue with MSAL in which the polling on the device code credential may end up triggering multiple polling requests and resolving them inappropriately. For more details, you can go here: AzureAD/microsoft-authentication-library-for-js#3476

In this PR, I made CI run these (playback) tests 20 times and the probability of failure seemed to increase.

Given that the MSAL bug is real, we should try skipping these tests to see if the core pipeline stops having the issue.

In the mean time, I've made a draft PR to MSAL with an approach that could solve the problem: AzureAD/microsoft-authentication-library-for-js#3553


There's one other small fix for a test. One ManagedIdentityCredential test was comparing times incorrectly.

@sadasant sadasant requested a review from HarshaNalluru April 28, 2021 17:48
@sadasant sadasant self-assigned this Apr 28, 2021
@ghost ghost added the Azure.Identity label Apr 28, 2021
@sadasant sadasant changed the title [Identity] Running node tests 20 times just to see if one fails with some consistency [Identity] Skipping device code tests on playback (live tests will help us release) Apr 29, 2021
@sadasant sadasant marked this pull request as ready for review April 29, 2021 12:44
@sadasant sadasant force-pushed the identity/device-code-test branch from 8cd694c to 215bcb9 Compare April 29, 2021 12:46
@sadasant sadasant requested a review from HarshaNalluru April 29, 2021 12:46
@ramya-rao-a
Copy link
Contributor

@sadasant Can you please update the PR description with a note on why the tests are being skipped?

@sadasant
Copy link
Contributor Author

@ramya-rao-a updated!

@sadasant
Copy link
Contributor Author

I believe I still saw a timeout issue even on this PR ... so I'll emphasize that I'm not certain this will fix the pipeline issue.

@HarshaNalluru
Copy link
Contributor

Was the 20 time run successful after the tests are skipped?

@sadasant
Copy link
Contributor Author

@HarshaNalluru I'll push that to see

@sadasant
Copy link
Contributor Author

Running these tests 20 times caused an error I wasn't expecting, only in one environment:

  1) ManagedIdentityCredential
       sends an authorization request correctly in an Azure Arc environment:

      AssertionError [ERR_ASSERTION]: 1619726 == 1619725
      + expected - actual

      -1619726
      +1619725

I'll investigate.

@sadasant
Copy link
Contributor Author

That last error seems like a bad test. I'll push a slight improvement. We were comparing two dates and assuming they may be slightly different, but sometimes they may get different a bit above our considerations.

@sadasant
Copy link
Contributor Author

sadasant commented Apr 29, 2021

We just had an issue installing dependencies in one of the pipelines. I don't know any other workaround than re-running the pipelines, so I'll wait until the pipeline finishes so I can re-run the item that failed.

@sadasant
Copy link
Contributor Author

@HarshaNalluru I won't re-start the CI job because it timed out here: https://dev.azure.com/azure-sdk/public/_build/results?buildId=869291&view=logs&j=889ff074-62b8-55e4-5d7f-907488e92b21&t=0da6ba07-cc57-557e-3355-438c1b2e5790&l=428

@sadasant
Copy link
Contributor Author

@HarshaNalluru I'll make a separate PR skipping Identity tests entirely, and I'll try to reproduce.

@sadasant
Copy link
Contributor Author

Since we were able to reproduce this without these tests, I'm testing some more aggressive skipping here: #15092

@sadasant sadasant closed this May 3, 2021
@sadasant sadasant deleted the identity/device-code-test branch May 3, 2021 18:40
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jul 1, 2021
Remove files from package which are not part of management plane SDK - update readme.md (Azure#15047)

* Update readme.md

* Update readme.md
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.

3 participants