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

Coherent IT config passing #16137

Draft
wants to merge 5 commits into
base: compatible
Choose a base branch
from
Draft

Conversation

martyall
Copy link
Member

@martyall martyall commented Sep 25, 2024

I came across this while working on a separate issue, but there are places within the integration-tests that we are not apply configuration consistently.

The way things work now

The way that config is built up and stored is sort of complicated, but basically the gist is:

  1. There is some base layer config that is shared everywhere in the integration tests (regardless of the "network", i.e. the manner in which they are run). It is extremely similar to Runtime_config.t
(* test_config.ml *)

(* It looks strange as a function, but this ends up being necessary given how other things are constructed *)
val default : constants:test_config.constants -> test_config.t
  1. Each individual test is allowed to patch this in its own way
(* e.g. payments_test.ml *)

val config : constants:test_config.constants -> test_config.t
  1. While the test is running, this base layer is modified and a runtime config value is created. The network expand function takes arguments test_config and constants and defines a runtime_config : Runtime_config.t
(* e.g. mina_docker.ml *)

(*
test_config == Some_test.config constants
*)
runtime_config = pick out fields from test_config...
  1. The test stores an updated constants : Test_config.constants value which is accessed in subsequent portions of the test. This is supposed to reflect all of the changes made to the initial configuration. It also stores a json representation of the constructed runtime_config value, presumably to load it in when the test boots up.
(* e.g. *)
...
; constants = some_function_to_apply_runtime_config runtime_config constants 
; runtime_config = Yojson.to_yojson runtime_config
...

This PR fixes...

  • In (2) above, we are not using exhaustive pattern matching on the test_config value, so the wildcard will discard overrides to runtime_config unless you remember to update it here as well (trust me, this can happen). I introduced exhaustive matching here to fix this. I made additional simplifications to the test_config structure RE proof_config. E.g. there were duplicate entries for block_window_duration_ms and transaction_capacity_log_2

  • In (3) above, we are discarding some runtime_conifg overrides for the when updating the constants. This is no longer the case.

@martyall martyall force-pushed the martin/coherent-it-config branch 2 times, most recently from b8f5e97 to 52d8ecc Compare September 25, 2024 07:38
@martyall martyall force-pushed the martin/coherent-it-config branch 3 times, most recently from 781ba91 to eab6fb2 Compare September 25, 2024 08:57
@martyall
Copy link
Member Author

!ci-nightly-me

@martyall
Copy link
Member Author

!ci-nightly-me

@martyall
Copy link
Member Author

!ci-nightly-me

@martyall martyall force-pushed the martin/coherent-it-config branch 2 times, most recently from c757bf2 to 6fbc7fe Compare September 25, 2024 19:26
@martyall martyall force-pushed the martin/coherent-it-config branch 2 times, most recently from 8a72934 to 7052864 Compare September 25, 2024 20:03
@martyall
Copy link
Member Author

!ci-nightly-me

Some Runtime_config.Proof_keys.Transaction_capacity.small
}
; work_delay = 1
; transaction_capacity_log_2 = 2
Copy link
Member

Choose a reason for hiding this comment

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

nit
i have a slight preference for modifying the type and using the variable.

Some Runtime_config.Proof_keys.Transaction_capacity.small
}
; work_delay = 1
; transaction_capacity_log_2 = 2
Copy link
Member

Choose a reason for hiding this comment

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

nit
slight preference for modifying type and changing transaction_capacity_log_2 to a the variable.

Some Runtime_config.Proof_keys.Transaction_capacity.small
}
; work_delay = 1
; transaction_capacity_log_2 = 2
Copy link
Member

Choose a reason for hiding this comment

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

nit
same as above

Some Runtime_config.Proof_keys.Transaction_capacity.medium
}
; work_delay = 1
; transaction_capacity_log_2 = 3
Copy link
Member

Choose a reason for hiding this comment

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

ditto

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