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

[Engineering] TokenResolver "should end polling" - All Builds Have the Same Failing Test #1787

Closed
mdrichardson opened this issue Feb 25, 2020 · 2 comments · Fixed by #1783
Closed
Assignees
Labels
P1 Painful if we don't fix, won't block releasing R9 Release 9 - May 15th, 2020
Milestone

Comments

@mdrichardson
Copy link
Contributor

mdrichardson commented Feb 25, 2020

All of the builds have one test that is currently failing with:

image

I believe this has something to do with the tests being run in Node v12; they pass with Node v11 and lower.

I'm still digging into why this is, but so far, I've found that:

In the test, we pass in a log here:

TokenResolver.checkForOAuthCards(adapter, context, activity, log);

Eventually, the log does get 'End Polling' pushed to it here:

if (log)
    log.push('End polling');
return;

However, log is an empty array by the time it gets back to the assert:

assert(log.indexOf('End polling') !== -1, 'did not end polling');

This works in the latest v11 of Node, v11.15.0. This breaks in the next release, which is the first of v12, v12.0.0. I wasn't able to find anything that stands out in the changelog.

This could probably be fixed if we move everything to async/await, but I'm not sure. There might be an easier way.

@mdrichardson mdrichardson changed the title All Build Tests Failing All Builds Have the Same Failing Test Feb 25, 2020
@Zerryth Zerryth changed the title All Builds Have the Same Failing Test [Engineering] TokenResolver "should end polling" - All Builds Have the Same Failing Test Feb 25, 2020
@Zerryth Zerryth added P1 Painful if we don't fix, won't block releasing R9 Release 9 - May 15th, 2020 labels Feb 25, 2020
@Stevenic
Copy link
Contributor

I found the issue. Race condition in the test

@stevengum
Copy link
Member

Will be fixed in #1783 via f1b6fc2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Painful if we don't fix, won't block releasing R9 Release 9 - May 15th, 2020
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants