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

test: stub/restore window.console properly #3861

Merged
merged 1 commit into from
Dec 15, 2016
Merged

test: stub/restore window.console properly #3861

merged 1 commit into from
Dec 15, 2016

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Dec 15, 2016

Description

We were accidentally stubbing console.log forever in our tests.

Specific Changes proposed

Move the storing of the default window.console to before we stub it out.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

brandonocasey
brandonocasey reviewed Dec 15, 2016
View reviewed changes
Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

Should we get rid of any additional console.logs that happen due to this change in this PR?

@gkatsev
Copy link
Member Author

gkatsev commented Dec 15, 2016

I can take a look at that.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 15, 2016

All the logging comes from addRemoteTextTrack. I'll make a separate PR to fix those.

brandonocasey
brandonocasey approved these changes Dec 15, 2016
View reviewed changes
Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

ok that works then, if you don't plan on making the PR now just make an issue and link it here

@gkatsev gkatsev mentioned this pull request Dec 15, 2016
Merged
@gkatsev
Copy link
Member Author

gkatsev commented Dec 15, 2016

Here's the other PR: #3864

@gkatsev gkatsev mentioned this pull request Dec 15, 2016
Merged
@gkatsev gkatsev merged commit 98b80df into master Dec 15, 2016
@gkatsev gkatsev deleted the fix-console-log-tests branch December 15, 2016 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonocasey brandonocasey brandonocasey approved these changes

Assignees
No one assigned
Labels
confirmed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gkatsev @brandonocasey