feat: Update GitHub functions #368
Conversation
eric-nguyen-cs
force-pushed
the
ericn/update-github-functions
branch
from
6098b6c
to
c84cdcf
Compare
| def test_import_from_github(client, github_mock, mocker): | ||
| # We mock the TaxonomyGraph.import_from_github method, | ||
| # which downloads the taxonomy file from a Github URL | ||
| mocker.patch("editor.api.TaxonomyGraph.import_from_github", return_value=True) | ||
|
|
||
| response = client.post( | ||
| "/test/testing_branch/import", | ||
| json={"description": "test_description"}, | ||
| ) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert response.json() is True |
There was a problem hiding this comment.
I find this test useful, at least we will see when it break.
Of course this test should have more checks:
- is project updated etc.
- eventually which methods were called on the mock
There was a problem hiding this comment.
Not really sure what value this tests brings, as the only difference with the /upload endpoint is that we have to access Github, which should be mocked
There was a problem hiding this comment.
I hear the first part of the video you mentioned in other comment. I'm ok with what is said in the video. I also value more integration test than unit test if functions are just assembling stuff.
But here, this is no unit test, this is integration test, that's why I think it's useful.
| def test_add_taxonomy_duplicate_branch_name(client, github_mock): | ||
| github_mock.return_value.get_repo.return_value.get_branches.return_value[0].name = "test_branch" | ||
|
|
||
| with open("tests/data/test.txt", "rb") as f: | ||
| response = client.post( | ||
| "/test_taxonomy_2/test_branch/upload", | ||
| files={"file": f}, | ||
| data={"description": "test_description"}, | ||
| ) | ||
|
|
||
| assert response.status_code == 409 | ||
| assert response.json() == {"detail": "branch_name: Branch name should be unique!"} |
There was a problem hiding this comment.
here also test is useful I think, here again we might add more checks.
There was a problem hiding this comment.
Same here, I believe this checks if the mock function works correctly and if we check for duplicate branches by using the get_all_branches endpoint of Github (that is not the case anymore, we check if the branch we want to create already exists or not).
My philosophy on tests follows quite closely what this engineer describes in this video: https://www.youtube.com/watch?v=MbU-PKukdMw
If that's ok, I'll merge the PR and if you want to add the test back, I'll do that in another PR
What
PyGithubwithGithubkitfor async HTTP requestsrun_in_threadpoolto run I/O-bound synchronous functions inside of asynchronous path operations/background tasks (if FastAPI detects that a path operation/background task is a co-routine, it directly runs the function in the main thread, instead of in a worker thread, which can block the server from responding if the co-routine contains blocking synchronous code - like the parsing function on big taxonomies) -> Fixes Github export, by usingAsyncTransactionContextinBackgroundTasks(this also allows forcontrollerscode reuse)project_controllersandproject_models)GtihubBranchAlreadyExistserror)Fixes bug(s)
Fixes #349