-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Updated search assertions on some search tests / throttle tests (mocks and assertions) #649
Conversation
- fixed search: more section title is not always empty, it can contain "Apps" - fixed missing comments (ds:9 => ds:8) - new url called to test throttle, - updated expected results
…will answer quickly
package.json
Outdated
@@ -27,7 +27,8 @@ | |||
"debug": "^3.1.0", | |||
"got": "^11.8.6", | |||
"memoizee": "^0.4.11", | |||
"ramda": "^0.21.0" | |||
"ramda": "^0.21.0", | |||
"sinon": "^15.2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a dev dependency, since is it's only for tests
this.timeout(15000); | ||
|
||
before(function () { | ||
server = sinon.fakeServer.create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add some comments explaining what this does and why we need it
test/utils.throttle.js
Outdated
interval: 5000 | ||
describe('Throttle tests', function () { | ||
let server; | ||
this.timeout(15000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I understand correctly, we're testing the throttle function generically here (outside of any integration with google play), which is good. But since we are in control, can you set the test up to work with lower waits? it's not desirable to have unit tests running for 15 seconds if we can help it.
Ideally you should update this test to work in < 1s, and remove the need for increasing this timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The throttle test used to call external api. If we don't want to rely on the availability of this particular api we can use mocks.
The fake server intercept http calls and return specified objects if it mach the same method/url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be extended to play store calls, but if we mocks responses we won't be able to see play store interface changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep it agnostic since the module is generic as well (ideally we wouldn't implement this inside this package, instead rely on another module #650 but I think at some point this was introduced because of a change in the requests library)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i also searched for throttle libraries, but i found libraries that were unable to handle both interval and number of requests. I'll try the p-trottle you mentionned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p-throttle needs esm migration. i'll try it after esm migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments
… interval and added comments on sinon usage.
… interval and added comments on sinon usage.
Throttle tests
Updated api calls to use mocks in throttle tests.
Updated expected delay between calls to be at least 4000ms when throttle delay is set to 5000ms.
When using throttle with 5000ms between 3 requests, the exact delay was sometime 4000 + 6000 or 5000 + 5000.
function setTimeout documentation: