-
Notifications
You must be signed in to change notification settings - Fork 84
Integration tests for support Linode integration with alerts #669
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
base: proj/aclp-linode-alerts
Are you sure you want to change the base?
Changes from 2 commits
2736865
e256458
2ca53cc
9d9834d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -235,6 +235,16 @@ def instance_type_condition(linode: Instance, type: str): | |||||||||||||||||||||||||||||
| return type in str(linode.type) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def test_get_linodes_verify_alerts(test_linode_client, create_linode): | ||||||||||||||||||||||||||||||
| linodes_list = test_linode_client.linode.instances().lists[0] | ||||||||||||||||||||||||||||||
| assert len(linodes_list) > 0 | ||||||||||||||||||||||||||||||
| assert linodes_list[0].alerts.cpu >= 0 | ||||||||||||||||||||||||||||||
| assert linodes_list[0].alerts.io >= 0 | ||||||||||||||||||||||||||||||
| assert linodes_list[0].alerts.network_in >= 0 | ||||||||||||||||||||||||||||||
| assert linodes_list[0].alerts.network_out >= 0 | ||||||||||||||||||||||||||||||
| assert linodes_list[0].alerts.transfer_quota >= 0 | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If not mistaken, values of |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
Comment on lines
+256
to
+264
|
||||||||||||||||||||||||||||||
| linodes_list = test_linode_client.linode.instances().lists[0] | |
| assert len(linodes_list) > 0 | |
| assert linodes_list[0].alerts.cpu >= 0 | |
| assert linodes_list[0].alerts.io >= 0 | |
| assert linodes_list[0].alerts.network_in >= 0 | |
| assert linodes_list[0].alerts.network_out >= 0 | |
| assert linodes_list[0].alerts.transfer_quota >= 0 | |
| linode = test_linode_client.load(Instance, create_linode.id) | |
| assert linode.alerts.cpu >= 0 | |
| assert linode.alerts.io >= 0 | |
| assert linode.alerts.network_in >= 0 | |
| assert linode.alerts.network_out >= 0 | |
| assert linode.alerts.transfer_quota >= 0 |
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 would implement extra filtering to find just created linode, but this test is like smoke test and in my opinion we don't need to check specific values. Do you see any benefit of this change?
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.
Can we add a test case to creat linode with aclp alerts? Adding system_alerts should be good
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.
Good point, please verify test just added: test_update_linode_aclp_alerts
Copilot
AI
Mar 31, 2026
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.
In test_linode_alerts_workflow, the test updates linode.alerts (cpu/io/network_*/transfer_quota) and calls linode.save(), but none of the subsequent assertions check that the update actually took effect. As written, the test would still pass even if the save was a no-op, which weakens the intended coverage. Consider asserting the saved values after a reload, and then align the clone assertions with the intended behavior (either the clone inherits the updated alerts, or it resets to defaults—right now it asserts the same default values as before the update).
Outdated
Copilot
AI
Mar 31, 2026
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 new_linode is not None: is redundant here: the test already dereferences new_linode.alerts.* above, so new_linode must be non-None for the test to reach the cleanup. Prefer unconditional deletion (or a try/finally around assertions) to make the cleanup intent clearer.
| if new_linode is not None: | |
| new_linode.delete() | |
| new_linode.delete() |
Outdated
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.
We can consider not testing this case, because this behavior will be removed in a following api change. I will send the doc to you.
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.
create_linodeis not used in this test. Probably can be removed.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.
Correct, but we need at least one linode to verify get linodes endpoint. It was suggested by copilot #669 (comment). What do you think about this?
Uh oh!
There was an error while loading. Please reload this page.
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.
The copilot suggests to not use the listing endpoint
linode.instance()and usecreate_linode. I agree that it's safer to not assume there is linode available by listing.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 use code fix suggested by Copilot. Should I change it somehow?