-
Notifications
You must be signed in to change notification settings - Fork 192
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
introduction of comprehensive test suite #131
base: master
Are you sure you want to change the base?
Conversation
hi @fleimgruber thanks for the work! I am going through the failings (using your branch): test_basic_queries for raw client: the one failing is the last one which is correct that it returns a zipfile. That test function only checks if response is string tbh I think it is good if we remove a lot of weird tests and strive to put all queries both pandas and raw in it and check if there is an appropriate response. checking actual data is a bit difficult a lot of work and not worth it I think. I will leave it up to you how you want to design that but that is my recommendation. I want to focus on some bug fixing so if you can set this up further it would be great. I have some experience with github actions so can set that up myself no problem. Thanks for your work so far! |
7b9c547
to
8ab69ab
Compare
I rebased and pushed the latest changes. The following notes reference the state before the rebase. Note that after the rebase we get new I tried to follow the README API examples as closely as possible. For some, I had to explicitly use keyword arguments, see also notes below. Would appreciate a review as this is not a small change. I also tried to give examples of how things are extensible using the pytest fixture mechanisms here and there. If you have any questions on the pytest concepts and usage (e.g. only run subsets of tests), let me know.
|
hi @fleimgruber first of all apologies for not seeing the work you did! I was kind of misguided by the more simpler title of your PR and also missed the notification. I noticed now because I get new notifications for other PR's. big thank you for trying your hand at this. Having a test suite would be great. I also got somebody else trying its hand at it this week, but yours is more comprehensive right now. I will try to run your fork locally to look at the tests and what goes wrong. I am not the original author of the library so some failures I may not know either but we shall see. I hope to have some time in the coming days for this. ps: i will update your title to better cover what the PR does |
hi @fleimgruber im going through the code, your setup looks very nice! I dont have a lot of experience with pytest but it works quite nice. Could you perhaps give me write access to your branch? I think that I can fix most things that are currently erroring. I also would like to check with the latest version of the library from the master branch just to be sure. an important thing I saw is that is very crucial which days we query in the test. some endpoints are not available for those days, but its a but clunky to change the day all over the place. I have for now disabled that test in my local copy as I dont see that as a true failure |
done.
Where does it need to be changed?
We could write tests that test the failure in these cases? |
8ab69ab
to
814cd63
Compare
FYI, I also just rebased |
814cd63
to
47c3372
Compare
FYI, just rebased again on master. Additional commits for
This is the pytest summary:
where I omitted all other cases of
|
47c3372
to
0c76333
Compare
hi @fleimgruber wow thanks a lot thats quite some work! I am sorry this got left on my todo list. so if I understand correctly it mostly works? Could you please share the exact calls you do for those three remaining errors? I will try to look into the pagination and general exceptions. for the http 500 ill send an email to the transparancy team itself, thats a server issue. Again thanks for your efforts and sorry for my delay! |
No worries, with holidays and all... One nice thing about pytest is that you can use the outputs of the test report to select and re-run the interesting failing cases (including their respective parametrization) individually; in this case e.g. like this:
from the repo root. You can then also debug those tests to see the actual API call that they use. |
@fboerman In light of my bug factory (#287 which spawned #292 #296 and #301) I think that maybe it's a good idea to get back to the idea of serious automated tests. Cached requests (as discussed in #83 ) and well maintained tests will not only allow for quicker iterations and fewer bugs but will also take some weight off your shoulders. I'd love to contribute but there are already 2 PRs and this is a big undertaking. Maybe you could chart the course? |
2f2f2f9
to
eb1e321
Compare
@fboerman I rebased and set up Github Actions for pytest to better talk about the remaining issues. I can split the CI related commits into a separate PR if you want to once the tests (this PR) are all green. Results on my repo are here: https://github.com/fleimgruber/entsoe-py/actions/runs/9085525870. To be clear, the test suite is composed of all example usages from the README. Remaining errors are:
For this to work on https://github.com/EnergieID/entsoe-py we need to set a secret for the API Key referenced here: https://github.com/fleimgruber/entsoe-py/blob/eb1e321c6bc5e3eefc1c605d3a9f308b6d365d4a/.github/workflows/ci.yaml#L10. |
hi @fleimgruber sorry for the delay, I am rather busy need to find some time to go through this more thoroughly |
@fboerman What about a pair-review via video call, as an efficient quickstart for looking into it yourself more thoroughly afterwards? |
eb1e321
to
db66a09
Compare
@fleimgruber I did not find enough time to go through thoroughly this myself so perhaps that is a good idea indeed. please shoot me an email at [email protected] to discuss how to set it up! |
db66a09
to
27a52f5
Compare
@fboerman rebased on latest master. |
refs #87.
Right now, 4/12 tests are failing:
I have not looked into those yet. Would appreciate a quick look by someone who can judge whether these tests are based on wrong assumptions regarding API results, are testing too deep into the implementation or are simply not relevant any more.
Additionally, I think a natural next step would be to have a Github Action for unit tests. I wanted to get familiar with Github Actions in any case so would be up for working on it as well. Please let me know if you want to have the Action also developed in this PR, a new PR or if you prefer to implement Actions yourself.