Fix build failure #105
Fix build failure #105
Conversation
…re-optimizing-test
- Removed call to api. - Added test to options handler instead
- Merge branch 'feature-optimizing-test' of https://github.com/JuanMaRuiz/psi into feature-optimizing-test - Removed api-response-test
…iz/psi into feature-optimizing-test
There was a problem hiding this comment.
Great tests 👍 🎉
I think I still like the idea of integration testing with the real API. Tbh it was a good test, it kept failing due to the API itself being flaky, which definitely helped me in diagnosing the underlying errors.
nit: changing the wording of the test methods
Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>
Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>
There was a problem hiding this comment.
I talked to another dev (hi @connorjclark) and I think we both agreed that the more simple solution to the monkey-patching of handleOpts and getThreshold would be to just modules.export them to test them directly without rewire. I think exporting would be better than adding another dependency.
Sorry for the 11th hour feedback 😃
|
No problem, I think is a better solution. The initial idea was not changing the original code too much, but I prefer your solution. Best regards. |
|
Pro 🙏 |
Although the coverage of psi is great
I think there are some minor changes that could be included in order to have more stable tests.
In the last PR we had some problems in the test, marking the build as instable, due a connection with the API. Another problem, related with travis and web requests was solved in the pass.
If we take a look to
api-response.test.jswe see that tests are:From my point of view we should avoid this call to the real API and test should check the methods instead.
This PR contains several changes that try to solve that.
Hope it helps.
Best regards.