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

Debounce spy #50

Merged
merged 5 commits into from
Sep 13, 2022
Merged

Debounce spy #50

merged 5 commits into from
Sep 13, 2022

Conversation

confused-Techie
Copy link
Member

@confused-Techie confused-Techie commented Sep 5, 2022

This debounce spyOn uses a decaffeinated spec/spec-helper.js to mock the implementation of debounce in underscore.

This is because on the older version of underscore we are using, it relies on Date().getTime() instead of Date.now() which is what we are mocking.

And thanks to @fabianfiorotto for determining this as the best solution for the problem.

With this change, it allows all tests in spec/atom-environment-spec.js and spec/config-spec.js to pass successfully. But it is important to note, that once we get underscore updated on pulsar-edit/underscore-plus this mock implementation likely will not be needed.

This PR requires PR #45 to be merged, since it contains the majority of decaffeination work done.

@confused-Techie confused-Techie linked an issue Sep 5, 2022 that may be closed by this pull request
@confused-Techie
Copy link
Member Author

While some of the tests are still running, the Ubuntu Tests are looking great

Editor: 24 Failed Tests
Package: 22 Failed Tests

@fabianfiorotto
Copy link
Contributor

I would put the mocked debounce function in its own file. Also, we should add some comments to the code about why it was needed and how we could remove it in the future.

@confused-Techie
Copy link
Member Author

I would put the mocked debounce function in its own file. Also, we should add some comments to the code about why it was needed and how we could remove it in the future.

Totally fair point, I'll go ahead and prettify this a bit more when home from work.

@confused-Techie
Copy link
Member Author

@fabianfiorotto I've implemented the changes you suggested, what do you think?

@fabianfiorotto
Copy link
Contributor

@confused-Techie it looks great

@fabianfiorotto
Copy link
Contributor

@confused-Techie, with this change, we can rollback any other change made to spec/config-spec.js

This commit was made originally to avoid timing while testing. But as thats now fixed this can be restored to original functionality.
@confused-Techie
Copy link
Member Author

@confused-Techie, with this change, we can rollback any other change made to spec/config-spec.js

Great catch. I've reverted those changes on this branch.

@mauricioszabo
Copy link
Contributor

LGTM 👍

@confused-Techie
Copy link
Member Author

Rad, so with approval, and having had a few eyes looking at this, and the tests looking great I'll go ahead and merge. Thanks to everyone here that's contributed!

@confused-Techie confused-Techie merged commit 62cad3c into master Sep 13, 2022
@confused-Techie confused-Techie deleted the debounce-spy branch September 18, 2022 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix config tests that check for debounces on the config.set
3 participants