search

apify / apify-client-python

Apify API client for Python
https://docs.apify.com/api/client/python/
Apache License 2.0
44 stars 11 forks source link

feat: add log() method to BuildClient (Python) #179

Closed tobice closed 6 months ago

tobice commented 6 months ago

This wraps the new /actor-builds/:build_id/log endpoint. See API reference for more information.

tobice commented 6 months ago

@fnesveda I haven't added any tests as the existing suite is very limited and it'd take some investment to extend it properly.

This is something I'd actually like to look into, to get the tests at least on par with apify-client-js, but it's probably not the biggest burning issue right now.

fnesveda commented 6 months ago

@fnesveda I haven't added any tests as the existing suite is very limited and it'd take some investment to extend it properly.

This is something I'd actually like to look into, to get the tests at least on par with apify-client-js, but it's probably not the biggest burning issue right now.

Yeah, that's OK, the tests here are pretty basic, so it would take quite a lot of effort to add new ones for specific endpoints. It would be nice to have proper tests for the client, but it's really not worth the effort right now.

When I was writing the Python API client, I decided not to replicate the test structure from the JS API client, because I didn't really like it - the unit tests basically have to mock the whole API server, which is a lot of duplicated code, and you have no guarantee that they're mocking it properly.

I was planning to come up with some proper testing framework, but that would mean rewriting half of the integration tests we have for the platform, and that seemed like a lot of effort too. And at that point we didn't know if the investment into Python tooling would be worth it, so we decided to not put in the effort into writing automated tests, and we just tested everything manually.

In the end, I think it was the right decision - the manual testing didn't take too much time, and we don't really do many structural changes to the API client that could cause bugs that only a full test suite in the API client would catch.

Anyway, I think the decision whether it's OK to keep the API client like this, with bare-bones tests, is now on @vdusek as the new owner of our Python tooling, what do you think?

If we ever decide to write proper tests here, I think we could take inspiration from the testing framework that's in the Apify SDK for Python, there the tests are much more detailed, and the test fixtures make writing tests quite easy, I would say.

vdusek commented 6 months ago

Hi there, improving the test suite for the Python client is currently not on our roadmap due to the main focus on Crawlee for Python. So it's up to you guys, if you could find time to implement some better tests here, it would indeed be awesome. Otherwise, just stay with the basic tests we currently have in place.

tobice commented 6 months ago

@fnesveda Added the async API variant 👍

tobice commented 6 months ago

Re tests, I generally don't feel comfortable submitting code without at least some test automation 😅 Manual testing is a valid option but also by definition brittle.

I also don't really have that much of a problem with mocking. For a unit test, that's fine. Just a piece of code that tells me that the new method calls the expected API endpoint and pipes back the data. For me as a new developer on the team who doesn't really understand how the client is built from within, this is already useful feedback.

With integration / e2e tests, while they are obviously preferred, maintaining a full suite on every single layer (API -> client -> SDK) is perhaps also not very economical.

Anyway, as said, this is definitely something I'd enjoy looking into (after agreeing on an approach with both of you), but it also doesn't seem like a burning issue right now.

tobice commented 6 months ago

Updated CHANGELOG.md.