Failed tests with default setup, on master branch

vkhomiv-hubspot commented 5 years ago

I decided to contribute here. After the download and setup, I run npm run build As a result, I see 102 passing 28 failing. Last few because of ten_secondly_rolling limit but all other because not matching expectations. It seems like tests are outdated and need some afford to sync with updated API. Also, we need to do something with requests throttling to avoid reaching the limit.

pcothenet commented 5 years ago

Hi @vkhomiv-hubspot, absolutely.

Some of the non-mocked tests were relying on data on the demo account that has moved / changed and I haven't had time to fix them all.

For the request throttling, how would you suggest implementing? Do you know if the secondly limit is available in the response or headers?

vkhomiv-hubspot commented 5 years ago

Hi @pcothenet, I will help you with tests. Also going to investigate possible approaches with call limits.

For now, I see a couple of issues with the demo account. HubSpot introduced/modified few endpoints from GET to POST, (Search for companies by domain POST /companies/v2/domains/:domain/companies as an example). And the demo account allows doing only GET requests, POST is prohibited, which failing a specific test for it. How to move on with I don't know yet.

Also, let's consider the case with tests in general. Currently, they are run against the real API via the demo account - which has few side-effects (call limits, POST is prohibited, changes on demo account which can impact test results, etc.) so I have two suggestions:

modify tests and mock API calls and have classical unit tests which will be testing wrapper itself, not the API
introduce few integration tests against the real API and special account where the data is blank and each test will have before steps to set the needed configuration for the test and after to rollback to the blank state (and each developer has to use own account for integration testing, which will solve issue with daily call limit issue)

What do you think about that?

pcothenet commented 5 years ago

100% agree. We had started to mock most things in the previous project back in December, but that takes quite a bit.

For integration tests: for our own usage, we use our own developer account for integration testing. I believe that's the right approach too. That also takes a bit of time setting tear-downs and stuff but should be the way to go.

For CI: HubSpot tends to retire those if inactive. Would you be able to set up a non-expiring developer account that we can use in CI?

MadKudu / node-hubspot

Failed tests with default setup, on master branch #195