search

OceanNetworksCanada / api-python-client

Provides easy access to ONC data in Python
https://oceannetworkscanada.github.io/api-python-client/
Apache License 2.0
10 stars 9 forks source link

Issue 2 fix test failure #5

Closed kan-fu closed 1 year ago

kan-fu commented 1 year ago

Fixes #2

I have a plan to add CI with Github Action, but it will be done in another issue after I add tox, flake8, black, and isort in #1.

I also have a long-term plan to rewrite tests in pytest.

Jacob-Stevens-Haas commented 1 year ago

Also are you on windows? It looks like the files are all committed in CRLF, I'm going to look into how to write a .gitattributes file to make line endings cross-platform.

aschlesin commented 1 year ago

Not sure about my role in code review as an outside collaborator, and let me know if I'm out of line, but LGTM with one suggestion.

@Jacob-Stevens-Haas - I think it would be great if you could be the reviewer on these changes done by Kan and team. That would be much apreciated. Thank you!

kan-fu commented 1 year ago

Also are you on windows? It looks like the files are all committed in CRLF, I'm going to look into how to write a .gitattributes file to make line endings cross-platform.

Yeah, I am using Windows for development. I will add a new commit for this line ending.

kan-fu commented 1 year ago

Thank you so much! Appreciate the robots readme - had figured it out once before, but this saved me a lot of time. All tests pass on my machine

Not sure about my role in code review as an outside collaborator, and let me know if I'm out of line, but LGTM with one suggestion.

Robot framework is not that intuitive for me. Always need to open a test file recording all the commands to run the test. That is why I plan to make the transition to pytest, which I assume is more common for python developers.

Jacob-Stevens-Haas commented 1 year ago

@Jacob-Stevens-Haas - I think it would be great if you could be the reviewer on these changes done by Kan and team. That would be much apreciated. Thank you!

Thanks, that's great to hear and I'd love to. I'll still defer to you guys to decide when to merge to master though :)

Yeah, I am using Windows for development. I will add a new commit for this line ending.

May not be necessary in this pull request, but found a github reference for this file. Should just be a quick add

Robot framework is not that intuitive for me. Always need to open a test file recording all the commands to run the test. That is why I plan to make the transition to pytest, which I assume is more common for python developers.

Agree completely :laughing:

kan-fu commented 1 year ago

There are two new test failures (9-12, 9-13) after yesterday's major release. They are caused by a pagination bug from our backend and I have reported it to the developer.

Other than that, I suspect that test 9.13 is wrong. I guess there should be more than 2 rows. I will double check it after the backend bug is fixed.

Jacob-Stevens-Haas commented 1 year ago

I see the failure now. What was given a "major release"?

Also, it may be more stable to test for the right number of columns, rather than rows. That way, as records get added, the test doesn't need to change.

kan-fu commented 1 year ago

I see the failure now. What was given a "major release"?

Also, it may be more stable to test for the right number of columns, rather than rows. That way, as records get added, the test doesn't need to change.

Our system (Oceans 3.0 Data Portal, we call it DMAS internally) is scheduled to be released every month. Here is the release note. There is a ticket called DMAS-78061 under bug section, in which we are trying to fix the rowLimit bug of archivefiles API endpoint. However the root cause is fundamental and a quick fix did not resolve all the issues.

Your point about testing with columns makes sense in most cases. But here test 9-12, 9-13 is about testing pagination, for example test 9-12 uses parameter with rowLimit=5 and allPages=True (I don't know the design here. Why not just query without rowLimit?), so the total rows should be equal to 15, which is the return of the API call (without token) without rowLimit. Test 9-13 (token removed) returns 582 files, that is why I have this suspicion about 9-13.

Most of the time the history data is not changed (otherwise it will cause trouble because they are used for academic research), so testing using history data should be stable and test failures actually expose bugs from both this library and the backend server, which is expected. BTW, I believe Issue #3 was fixed on the server side.

One last question, do you think my last commit is fine in terms of your suggestion?

Jacob-Stevens-Haas commented 1 year ago

The monthly "major" API change feels pretty odd, but I'm coming from the package/library world where semantic versioning is de rigueur rather than the web API world, which I don't understand as well. Has your web team heard of semver? Is the decision to not follow semver intentional?

IMO it would help get this library out of the business of "test failures actually expose bugs from both this library and the backend server", which means that CI will randomly fail, blocking library development while DMAS is fixed.

kan-fu commented 1 year ago

The monthly "major" API change feels pretty odd, but I'm coming from the package/library world where semantic versioning is de rigueur rather than the web API world, which I don't understand as well. Has your web team heard of semver? Is the decision to not follow semver intentional?

IMO it would help get this library out of the business of "test failures actually expose bugs from both this library and the backend server", which means that CI will randomly fail, blocking library development while DMAS is fixed.

These are all my personal understanding: this versioning (maybe called Calendar Versioning?) is mainly used for our DMAS system, and API is just one part of the monolithic backend. We don't expect to break a bunch of features every month. In fact, you can treat the major release in DMAS -> minor in semver, and minor release in DMAS -> patch in semver, so it actually follows semver to some extent. The choice was made a long time ago, but for this repo, the most popular semver will be used.

During my fixing the tests, I indeed thought about mocking the http request (possibly using response library) so running the tests will be much faster and independent from changes in the backend, which exactly follows your comment. But there are some difficulties like mocking up file download and extra efforts to maintain the mockup. I can bring it up in our internal meeting, but it is not a small decision so there is no guarantee about it.

What I can do is to integrate the tests in this library into our DMAS release cycle so it should reduce the chance of causing such bugs.

Jacob-Stevens-Haas commented 1 year ago

Ah, that all makes sense and helps my understanding. I have no experience with mocking server responses, and you're right to consider the maintenance burden. Adding these tests to the DMAS release cycle would help.