-
Notifications
You must be signed in to change notification settings - Fork 191
Add some code coverage for SPO connector to be able to backport/merge stuff again #3795
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
Add some code coverage for SPO connector to be able to backport/merge stuff again #3795
Conversation
| assert isinstance(client.graph_api_token, EntraAPIToken) | ||
| assert isinstance(client.rest_api_token, EntraAPIToken) |
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.
Do we want to add other checks too like that it has the expected _certificate / _scope / _private_key?
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.
If I'd test it properly, I'd actually test behaviour - pass certs and private key and verify that requests contain them, but the code is already quite a mess, I wanted to have something somewhat working to be able to unblock merges - now we're at 91.99/92% coverage and CI started to inconsistently fail
|
|
||
| download_result = source.download_function(drive_item, max_drive_item_age) | ||
|
|
||
| assert download_result is not None |
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.
I guess there's already an existing pattern, but would we be able to make a more precise assertion?
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.
I'd again make the assertion behavioral - rather than just checking the download function, check that using it is producing an expected API call to 3rd-party, but I don't think we've done things like that yet. Our testing codebase is pretty messy tbh 😬
… stuff again (#3795) Subj.
💔 Failed to create backport PR(s)
Successful backport PRs will be merged automatically after passing CI. To backport manually run: |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…t/merge stuff again (#3795) (#3799) # Backport This will backport the following commits from `main` to `8.19`: - [Add some code coverage for SPO connector to be able to backport/merge stuff again (#3795)](#3795) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)
…/merge stuff again (#3795) (#3797) # Backport This will backport the following commits from `main` to `9.2`: - [Add some code coverage for SPO connector to be able to backport/merge stuff again (#3795)](#3795) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)
…/merge stuff again (#3795) (#3798) # Backport This will backport the following commits from `main` to `9.0`: - [Add some code coverage for SPO connector to be able to backport/merge stuff again (#3795)](#3795) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)
…t/merge stuff again (#3795) (#3800) # Backport This will backport the following commits from `main` to `8.18`: - [Add some code coverage for SPO connector to be able to backport/merge stuff again (#3795)](#3795) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)
…/merge stuff again (#3795) (#3796) Backports the following commits to 9.1: - Add some code coverage for SPO connector to be able to backport/merge stuff again (#3795) Co-authored-by: Artem Shelkovnikov <[email protected]>
Subj.