Skip to content

Integration tests for support Linode integration with alerts#669

Open
psnoch-akamai wants to merge 3 commits intolinode:proj/aclp-linode-alertsfrom
psnoch-akamai:TPT-4269-python-sdk-qa-support-linode-integration-with-alerts
Open

Integration tests for support Linode integration with alerts#669
psnoch-akamai wants to merge 3 commits intolinode:proj/aclp-linode-alertsfrom
psnoch-akamai:TPT-4269-python-sdk-qa-support-linode-integration-with-alerts

Conversation

@psnoch-akamai
Copy link
Copy Markdown
Contributor

📝 Description

Integration tests for support Linode integration with alerts

✔️ How to Test

make TEST_CASE=test_get_linodes_verify_alerts test-int
make TEST_CASE=test_linode_rebuild test-int
make TEST_CASE=test_linode_alerts_workflow test-int
make TEST_CASE=test_try_to_update_linode_alerts_legacy_and_aclp_at_the_same_time test-int

@psnoch-akamai psnoch-akamai requested a review from a team as a code owner March 17, 2026 19:05
@psnoch-akamai psnoch-akamai requested review from ezilber-akamai and mawilk90 and removed request for a team March 17, 2026 19:05
@psnoch-akamai psnoch-akamai changed the base branch from dev to proj/aclp-linode-alerts March 17, 2026 19:08
@psnoch-akamai psnoch-akamai added the testing for updates to the testing suite in the changelog. label Mar 17, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds integration test coverage for Linode Instance “alerts” behavior in the Python SDK, focusing on listing, rebuilding, updating, cloning, and validating error behavior when mixing legacy and ACLP alerts.

Changes:

  • Add a new integration test to verify alerts are present when listing Linodes.
  • Extend the rebuild integration test to assert alerts fields are populated.
  • Add workflow/error-path integration tests for updating alerts and validating legacy vs ACLP alert incompatibilities.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@psnoch-akamai psnoch-akamai marked this pull request as draft March 19, 2026 13:07
@psnoch-akamai psnoch-akamai changed the title Tpt 4269 python sdk qa support linode integration with alerts Integration tests for support Linode integration with alerts Mar 19, 2026
@psnoch-akamai psnoch-akamai marked this pull request as ready for review March 30, 2026 17:17
return type in str(linode.type)


def test_get_linodes_verify_alerts(test_linode_client, create_linode):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create_linode is not used in this test. Probably can be removed.

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Contributor

@yec-akamai yec-akamai Mar 31, 2026

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 use create_linode. I agree that it's safer to not assume there is linode available by listing.

new_linode.delete()


def test_try_to_update_linode_alerts_legacy_and_aclp_at_the_same_time(
Copy link
Copy Markdown
Contributor

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.

assert linode.status == "running"


def test_linode_alerts_workflow(test_linode_client, create_linode):
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds integration coverage to validate Linode “alerts” behavior is present and consistent across common instance operations in the SDK’s integration test suite.

Changes:

  • Add an instances-list test that asserts alerts fields are present and non-negative.
  • Extend the rebuild integration test to assert alerts are accessible during rebuild.
  • Add workflow tests for reading/updating alerts, cloning behavior, and an error case when mixing legacy + ACLP alerts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +239 to +247
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


Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_get_linodes_verify_alerts is non-deterministic: it creates a Linode via the create_linode fixture but then asserts against instances().lists[0][0], which may be an unrelated instance (ordering/pagination is not guaranteed). To make the test reliable, assert against the specific Linode created by the fixture (e.g., load by create_linode.id or filter the list for that ID), and avoid reaching into the internal .lists cache when plain indexing/iteration works.

Suggested change
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

Copilot uses AI. Check for mistakes.
Comment on lines +381 to +404
linode.alerts = {
"cpu": 50,
"io": 6000,
"network_in": 20,
"network_out": 20,
"transfer_quota": 40,
}
linode.save()

wait_for_condition(10, 100, get_status, linode, "running")
new_linode = retry_sending_request(
5,
linode.clone,
region=linode.region.id,
instance_type=linode.type.id,
label=get_test_label(),
)
assert new_linode.alerts.cpu == 90
assert new_linode.alerts.io == 10000
assert new_linode.alerts.network_in == 10
assert new_linode.alerts.network_out == 10
assert new_linode.alerts.transfer_quota == 80
assert isinstance(new_linode.alerts.system_alerts, list)
assert isinstance(new_linode.alerts.user_alerts, list)
Copy link

Copilot AI Mar 31, 2026

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).

Copilot uses AI. Check for mistakes.
Comment on lines +406 to +407
if new_linode is not None:
new_linode.delete()
Copy link

Copilot AI Mar 31, 2026

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.

Suggested change
if new_linode is not None:
new_linode.delete()
new_linode.delete()

Copilot uses AI. Check for mistakes.
…ame_time and add test test_update_linode_aclp_alerts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing for updates to the testing suite in the changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants