-
Notifications
You must be signed in to change notification settings - Fork 131
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
Integration Tests for Borg #1716
Conversation
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
I'm facing a weird issue with testing. This is the test with removed mock command:
Running the command on its own produces this error:
But it works fine if I put any test above it:
Any clue what could be the cause? I did see that causing signal is disconnected on teardown in conftest.py |
@real-yfprojects Can I move existing tests to |
My supposition would be that this error is somehow triggered by emitting |
Signed-off-by: Chirag Aggarwal <[email protected]>
Pushed new integration tests. Tests will fail from this commit onwards. I've cleaned up the print statements for this commit but there are several wait and debug statement like |
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
This reverts commit 077f993.
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you have finished a test file pls add short docstrings to the functions and one docstring at the top of the file explaining what is being tested.
I was wondering what happens when a borg command fails (return code > 0) will the tests fail too? |
Signed-off-by: Chirag Aggarwal <[email protected]>
No, they won't directly fail. All tests are currently relying on vorta to show success message or some behavior and have assertions or wait for the same. I tested this earlier with borg check and although borg command failed, test passed since Vorta still displayed completed message. Only test_borg.py which directly calls Borg Jobs tests for returncode. Perhaps I can go through messages I'm testing and see if they internally are checking for returncode? |
Signed-off-by: Chirag Aggarwal <[email protected]>
…integration tests Signed-off-by: Chirag Aggarwal <[email protected]>
As long as there is a test catching the error it's fine I think. However if you find a simple solution for catching the error in the other tests that wouldn't be bad either. |
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
I checked out this branch and ran the tests on M1 Mac. Slightly modified command to work on arm64 architecture:
v1.1.18 Failed with: v1.2.2: SUCCESS v1.2.4 and 2.0.0b5 failed with: I think the fails are due to m1 Mac issues and not the tests themself. IIRC I've gotten the |
Yes, likely more related to Homebrew libs than our code here. What happens if you install and run those Borg versions via pip? Or if you point it to openssl 1.1 (should be in the Homebrew folder too). |
Pointed it to openssl 1.1 and now all versions of Borg passed except v1.1.18, with same error as before. |
Are you able to use v1.1.18 normally (via pip installation)? |
Unfortunately no, I believe I always receive the same error. But it is (maybe?) a good thing, because it means the issue lies with my system and not your code :) |
What's missing for this PR to be merged? |
Minor conflict, but otherwise we should merge it. |
Signed-off-by: Chirag Aggarwal <[email protected]>
Signed-off-by: Chirag Aggarwal <[email protected]>
|
Is this ready to merge? You can add tests for new features when adding those features. One CI test timed out (looks like a Github issue) |
Yes, ready to merge from my side. Which test timed out? Last commit seems to show all passed. |
One had a timeout and I restarted it this morning. Didn't look related to the code here. |
👍 |
Congratulations for this very big PR @jetchirag. 🎉 You put lots of work into it and it is very worth it since we can now, for the first time, be sure that PRs run with actual borg versions. |
@real-yfprojects Thank you! This was a big learning experience for me. |
Congrats on merging this beast! 👏 |
Tests take a good amount of time now. Is it possible to run some of them on-demand only and not for each commit? And only run a sensible selection for each commit? @jetchirag I still want to run all tests before doing a final merge, just not for each commit. For the macOS binary, we have an on-demand workflow that can be triggered by the user. |
On the plus-side, the tests seem to be fairly stable. We used to have some issues with race-conditions, but they seem less now. |
@m3nu I assume it would be through |
Let's discuss this further in #1778. BTW: After merging some PRs this morning, I'm still waiting for tests after 30 min or so. |
Related Issue
Fixes #1711
Motivation and Context
https://github.com/borgbase/vorta/wiki/Google-Summer-of-Code-2023-Ideas#test-on-live-borg-binary
Types of changes
Checklist:
I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.