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

Do not clobber console.log to spy on it #782

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

raphinesse
Copy link
Contributor

Fix for the problem described in #778 (comment)

Properties that are replaced with spyOn are restored after each test. The manually set spies that existed previously clobbered the console.log property permanently. This broke the test output with the new reporter.

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

LGTM and it seems to solve the problem if I introduce a test failure in my workarea.

@raphinesse do you think we need to make this kind of fix in other Cordova packages?

@raphinesse
Copy link
Contributor Author

do you think we need to make this kind of fix in other Cordova packages?

The tests are often quite poorly written, so I wouldn't rule it out. But I would probably just fix these kinds of things when they pop up instead of starting to hunt for it now. 🤷‍♂️

@raphinesse
Copy link
Contributor Author

Curiously, the previous code should never have caused any problems. See the last section Dot notation in the rewire docs. 😕 So I'll take back my poorly written code judgement from above 😥

Would be good to know what is going on here. I think I wrote similar code in other parts of Cordova.

@brodycj
Copy link
Contributor

brodycj commented Jul 16, 2019

Interesting. I would still favor merging this change since it at least seems to be cleaner and does seem to solve the observed problem for me.

It would certainly be nice if we can understand what was going on, though.

@raphinesse raphinesse merged commit 47c6048 into apache:master Jul 16, 2019
@raphinesse raphinesse deleted the proper-log-spies branch July 16, 2019 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants