[IMP] CONTRIBUTING.rst: Improve testing flow for external API calls #131
Conversation
There was a problem hiding this comment.
Separating repositories is not an option. You are not going to have a delivery-carrier-ctt or similar repository. That's a nonsense. And the interference is minimal. We have way more problems due to library versions, architecture things, glitches, etc, than this.
So I don't agree with this.
If you want to do integration tests, that's the only viable one.
That's a point of view. I don't agree on that. Tests should be always reproducible. That's not the case with your implementation.
Maybe but that is more global problems that are fixed once. Here, this is very very random. |
|
Here we have a philosophical debate, as you consider different things from us for tests. We consider that tests should check the real thing as expected by the user. You consider an ideal thing that may not be the same as the reality. And the random thing, it's true, but it's very unfrequent, which is the key. And when happens, it serves for notifying/taking other actions. Unrelated contributors receive a bit of inconvenience, but it's very transient, and you know that I'm not a passive agent that don't react quickly. |
You are lucky to be able to act on external providers...
At each time I touch something in delivery-carrier repository it happens (almost all build since yesterday are red...). So I'm very unfortunate... |
|
It's not red right now since this morning. |
Are you sure ? https://github.com/OCA/delivery-carrier/actions/runs/7288283480/job/19860574284?pr=643 |
There was a problem hiding this comment.
Agree with @pedrobaeza here. It's a tradeoff :
-
with mock :
- tests works allways. 👍
- Tests doesn't test if the module works. The API can totally fail and the test are still green. 👎
-
without mock :
- test fails if API is not available / is unstable. 👎
- test fails if API changed. 👍
- less work to develop 👍
If a module make test failing because of API calls. 2 options :
- fix module quickly.
- if not, disable test, and let maintainers fix the issue.
Ouch, I have merged this morning one PR: OCA/delivery-carrier#746, so they are still performing changes, but the goal is achieved: we check this for possible API changes. I'm looking to see if we need to make changes in the module, as it seems now there's a required parameter. |
|
@legalsylvain If module is alone in its repo it does not harm anything, so I agree in that condition. The problem appears when it impeach the flows of others. Two major drawbacks I see:
Such behavior refrain contributions and should be avoided. |
|
IMO, the goal is not achieved since it prevent a smooth review process of PR not related to randomly failing addons. We can argue at length about whether or not it's appropriate to depend on an external api for a test. In my opinion, however, it's important to ensure that whatever the decision, it doesn't introduce additional constraints for developers of other modules in the same repository. In this sense, I think that isolating these modules in separate repos would offer a number of advantages. In addition to avoiding this Russian roulette at every PR or merge attempt, it would then be possible to set up a mechanism periodically testing the module even if no changes are made to it. This would enable maintainers to be notified as soon as the api used is no longer compatible with the module using it, without causing problems for developers of other modules. |
|
Indeed. @sbidoul I don't know if it is possible on github to have some crons to test an API ? |
|
Could you point real PR(s) where such problem occures ? |
https://github.com/OCA/delivery-carrier/actions/runs/7645436922/job/20848863118?pr=764 https://github.com/OCA/delivery-carrier/actions/runs/7695289245/job/20967833989?pr=764 https://github.com/OCA/delivery-carrier/actions/runs/7651351244/job/20848976535?pr=765 Moreover, they now did a specific PR to test the API going back and forth opening and closing... OCA/delivery-carrier#763 |
|
I can only agree with the additional repo, on my side, It is not rare that I meet this issue when working on delivery-carrier repo. In a ideal world it is nice to test the real external api of course, but in reality the most frequent issue when tests fail is the unavailability of such api, who does it really help ? The failing tests don't point to any issue than need fixing, they just need to be re-run and have some luck ! |
|
Hello, for information in OCA/l10n-brazil we had a lot of issues external API's like this. A pragmatic way we worked around it is with this test decorator: https://github.com/OCA/l10n-brazil/blob/14.0/l10n_br_fiscal/tests/test_ibpt.py#L32 So as you can see (here for instance https://github.com/OCA/l10n-brazil/blob/14.0/l10n_br_fiscal/tests/test_ibpt_service.py#L35 ) we have a bit of the best worlds: we test against the external API only once a week (can be configured) and mock the tests the other days. Thus we never get the tests stuck for days but we still get a warning in just a few days if there is a change in the external service. The only issue we have is that the test coverage will change bit from days to days. That is often have PR's which aren't entirely green because there is say 0.05% less test coverage (and of course we merge them anyway in this case). Not sure it is the silver bullet, but may be this kind of work around can cut in for some of your use cases. |
That's an optimistic decorator that implies API does not fail at least once a day 😅 |
@sbejaoui @pedrobaeza @lmignon