Skip to content
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

Control plane testing overhaul #7

Merged
merged 46 commits into from
Dec 5, 2024
Merged

Control plane testing overhaul #7

merged 46 commits into from
Dec 5, 2024

Conversation

roshanrags
Copy link
Member

@roshanrags roshanrags commented Nov 29, 2024

Complete overhaul of control plane testing infrastructure with the goal of

  • easier to write tests
  • clearer tests
  • more complete tests (exhaustive checks)

Summary of changes:

  • Common test runner so tests are more clear and concise without noise
  • Outcomes are compared as vectors so it checks count, order and all outcome fields exhaustively
  • SystemContext to wrap the start time global so tests have separate instances and run without interfering with each other
  • UpdateEnclaveImageOutcome to track update_enclave_image calls explicitly
  • Randomness replaced with deterministic derivations
  • Some comments calling out (to me) surprising behaviour

Tests themselves and coverage can be improved as well but that's for another day.

@roshanrags roshanrags requested a review from vg-27 November 29, 2024 07:38
&test::get_gb_rates(),
&Vec::from([
allowed_regions: vec!["ap-south-1".to_owned()],
address_whitelist: vec![
"0x0000000000000000000000000f5f91ba30a00bd43bd19466f020b3e5fc7a49ec".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

we can parameterize this address during test setup. And provide the same to test::getLogs(). It was't clear what value should this address match to, since getLogs uses some default address.

Copy link
Member Author

Choose a reason for hiding this comment

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

get_log is actually quite messy, it uses the hardcoded address for everything from emitting contract to sender to depositors, should probably rework that

Copy link
Member Author

Choose a reason for hiding this comment

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

done

let log = test::get_log(Action::Open,
Bytes::from(("{\"region\":\"ap-south-1\",\"url\":\"https://example.com/enclave.eif\",\"instance\":\"c6a.xlarge\",\"memory\":4096,\"vcpu\":2}".to_string(),31000000000000u64,31000u64,market::now_timestamp().as_secs()).abi_encode_sequence()),
Bytes::from(("{\"region\":\"ap-south-1\",\"url\":\"https://example.com/enclave.eif\",\"instance\":\"c6a.xlarge\",\"memory\":4096,\"vcpu\":2}".to_string(),31000000000000u64,31000u64,0).abi_encode_sequence()),
B256::ZERO);
let address_whitelist = vec![
"0x0000000000000000000000000f5f91ba30a00bd43bd19466f020b3e5fc7a49ec".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

here also address can be parameterized.

let job_logs: Vec<(u64, Log)> = vec![
(0, Action::Open, ("{\"region\":\"ap-south-1\",\"url\":\"https://example.com/enclave.eif\",\"instance\":\"c6a.xlarge\",\"memory\":4096,\"vcpu\":2}".to_string(),31000000000000u64,31000u64,market::now_timestamp().as_secs()).abi_encode_sequence()),
let logs = vec![
(0, Action::Open, ("{\"region\":\"ap-south-1\",\"url\":\"https://example.com/enclave.eif\",\"instance\":\"c6a.xlarge\",\"memory\":4096,\"vcpu\":2}".to_string(),31000000000000u64,31000u64,0).abi_encode_sequence()),
// instance type has also been updated in the metadata. should fail this job.
Copy link
Member

Choose a reason for hiding this comment

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

remove this comment, there is not change in instance type. Also, in this test, instance should be running instead of failing since there isn't any update, please check.

Copy link
Member Author

@roshanrags roshanrags Dec 5, 2024

Choose a reason for hiding this comment

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

comment is wrong, removed. Behaviour is correct though, matches the code which exits if there is no change. Good q as to whether that is desirable, but out of scope for this PR IMO.

vg-27
vg-27 previously approved these changes Dec 5, 2024
@vg-27
Copy link
Member

vg-27 commented Dec 5, 2024

LGTM

Copy link
Member

@vg-27 vg-27 left a comment

Choose a reason for hiding this comment

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

LGTM

@roshanrags roshanrags merged commit 1f18f08 into master Dec 5, 2024
@roshanrags roshanrags deleted the roshan/cp branch December 5, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants