Skip to content

Conversation

nullie
Copy link

@nullie nullie commented Dec 14, 2018

For #2571

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)

@msftclas
Copy link

msftclas commented Dec 14, 2018

CLA assistant check
All CLA requirements met.

@d3r3kk
Copy link

d3r3kk commented Dec 14, 2018

Thanks for the PR @nullie, and the mention @iansan5653. Looking at your work now.

@d3r3kk d3r3kk self-requested a review December 14, 2018 17:42
@d3r3kk
Copy link

d3r3kk commented Dec 14, 2018

Seems good but we have some unrelated failures in tests at the moment. I'm going to add some code to quell those for the moment...

Copy link

@d3r3kk d3r3kk left a comment

Choose a reason for hiding this comment

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

Looks fine, and fixes the issue at hand. Thanks @nullie!

@d3r3kk
Copy link

d3r3kk commented Dec 14, 2018

Taking the fix as it does address the runtime problem.

However, it also re-enables a flaky test that has been pushed to a high priority item for our team this sprint.

@d3r3kk d3r3kk merged commit b0a54b5 into microsoft:master Dec 14, 2018
d3r3kk pushed a commit that referenced this pull request Dec 14, 2018
@nullie
Copy link
Author

nullie commented Dec 14, 2018

That was quick, I've actually made some mistakes in this and want to correct and refactor

@iansan5653
Copy link

He acted quickly because your PR addressed a really widespread problem (#3700). Probably should just submit another PR with revisions

@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants