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.

Sign up for GitHub

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

Jump to bottom

[test_loop] Cleanup old test loop code #11525

Merged
merged 10 commits into from
Jun 13, 2024
Merged

[test_loop] Cleanup old test loop code #11525

merged 10 commits into from
Jun 13, 2024

Conversation

shreyan-gupta
Copy link
Contributor

@shreyan-gupta shreyan-gupta commented Jun 8, 2024 •
edited
Loading

Part 6

🎉🎉🎉🎉🎉

Nothing more staisfying than seeing this
image

Other PRs

Part 1: [test_loop] Introduce TestLoopV2
Part 2: [test_loop] Change ClientQueries trait requirements in TestLoop util
Part 3: [test_loop] test_loop_sync_actor_maker implementation in TestLoopV2
Part 4: [test_loop] Introduce TestLoopPeerManagerActor for handling network messages across clients
Part 5: [test_loop] Convert current tests to use TestLoopV2
Part 6: [test_loop] Cleanup old test loop code
Part 7: [test_loop] Better visualizer support for TestLoopV2
Part 8: [test_loop] Simple TestLoopEnvBuilder

@codecov Codecov
Copy link

codecov bot commented Jun 8, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.44%. Comparing base (a9ca94b) to head (f2170ad).

Files Patch % Lines
chain/client/src/client_actor.rs 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #11525      +/-   ##
==========================================
+ Coverage   71.19%   71.44%   +0.25%     
==========================================
  Files         794      783      -11     
  Lines      160944   159863    -1081     
  Branches   160944   159863    -1081     
==========================================
- Hits       114578   114211     -367     
+ Misses      41373    40674     -699     
+ Partials     4993     4978      -15
Flag Coverage Δ
backward-compatibility 0.23% <0.00%> (-0.01%) ⬇️
db-migration 0.23% <0.00%> (-0.01%) ⬇️
genesis-check 1.35% <0.00%> (+<0.01%) ⬆️
integration-tests 37.66% <93.93%> (+0.25%) ⬆️
linux 68.87% <90.90%> (+0.24%) ⬆️
linux-nightly 70.93% <96.96%> (+0.24%) ⬆️
macos 52.48% <9.09%> (+0.09%) ⬆️
pytests 1.59% <0.00%> (+0.01%) ⬆️
sanity-checks 1.38% <0.00%> (+<0.01%) ⬆️
unittests 66.18% <18.18%> (+0.20%) ⬆️
upgradability 0.28% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This was referenced Jun 10, 2024
Merged
Merged
Merged
Merged
Merged
Merged
@shreyan-gupta shreyan-gupta marked this pull request as ready for review June 10, 2024 17:52
@shreyan-gupta shreyan-gupta requested a review from a team as a code owner June 10, 2024 17:52
github-merge-queue bot pushed a commit that referenced this pull request Jun 11, 2024
[test_loop] Change ClientQueries trait requirements in TestLoop util (#…
71e5352 
…11520)

Part 2

Change in interface of ClientQueries to not depend on
`AsRef<AccountId>`. Note that we can get the account_id from Client
using the validator signer module.

### Other PRs

[Part 1](#11521): [test_loop]
Introduce TestLoopV2
[Part 2](#11520): [test_loop]
Change ClientQueries trait requirements in TestLoop util
[Part 3](#11522): [test_loop]
test_loop_sync_actor_maker implementation in TestLoopV2
[Part 4](#11523): [test_loop]
Introduce TestLoopPeerManagerActor for handling network messages across
clients
[Part 5](#11524): [test_loop]
Convert current tests to use TestLoopV2
[Part 6](#11525): [test_loop]
Cleanup old test loop code
[Part 7](#11528): [test_loop]
Better visualizer support for TestLoopV2
@shreyan-gupta shreyan-gupta mentioned this pull request Jun 11, 2024
Merged
pugachAG
pugachAG approved these changes Jun 12, 2024
View reviewed changes
Copy link
Contributor

@pugachAG pugachAG left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉🎉🎉

Revert "[test_loop] ClientQueries trait to use Borrow<Client> instead…
149b658 
… of AsRef<Client> + AsRef<AccountId>"

This reverts commit 8c33367.
github-merge-queue bot pushed a commit that referenced this pull request Jun 12, 2024
[test_loop] Introduce TestLoopV2 (#11521)
0c09325 
Part 1

This PR has the set of major test loop infrastructure changes

## data.rs

This is the new implementation of storage of data on test loop. The way
it works is, we store any arbit data in a vec of type Any data and
return a handle for the data stored.

The handle is a lightweight "pointer" to the data (basically stores the
index) that can be easily passed around. This effectively acts like a
smart pointer to access specific data.

We also have the ability to register actors which create a "sender" out
of the actor handler and return it.

Why was this needed? For managing ownership of data like client_actor,
shards_manager_actor etc. We need to create "senders" for our actor
handlers and they need to be lightweight, cloneable, implement Send +
Sync for passing around threads etc.

Alternatives? The most obvious one that I had tried first was to use
Arc<Mutex<Actor>> but that was causing some concurrency related issues
and wasn't an elegant solution. Additionally, only the senders then had
access to the actor data which wasn't easily scalable for things like
adhoc events etc.

## sender.rs

TestLoopSender is a nice wrapper on top of actors that implements the
`CanSend<Msg>` interface. This pattern is similar to what we have for
actix wrapper (core/async/src/actix_wrapper.rs) for actors.

TestLoopSender needs to implement `DelayedActionRunner` as well for
things to function. We don't need to go too much into details of what
this is, but it's effectively used by the actor to schedule events
sometime in the future.

Link to old TestLoopSender PR:
#11450

## futures.rs

Newer implementation of future execution for TestLoopV2 using callbacks
as the main event. The overall logic for how things work is still the
same.

I had to do some playing around with FutureSpawner but nothing much has
changed functionality and implementation wise.

## test_loop.rs

This file implements TestLoopV2
- Got rid of the builder as we don't need it.
- "Events" are now of only one type, the CallbackEvent
- `future_spawner`, `async_computation_spawner` are exposed by the
testloop that can be passed to actors.
- `register_actor` to store actor to test_loop_data and returns sender.

## Other PRs

[Part 1](#11521): [test_loop]
Introduce TestLoopV2
[Part 2](#11520): [test_loop]
Change ClientQueries trait requirements in TestLoop util
[Part 3](#11522): [test_loop]
test_loop_sync_actor_maker implementation in TestLoopV2
[Part 4](#11523): [test_loop]
Introduce TestLoopPeerManagerActor for handling network messages across
clients
[Part 5](#11524): [test_loop]
Convert current tests to use TestLoopV2
[Part 6](#11525): [test_loop]
Cleanup old test loop code
[Part 7](#11528): [test_loop]
Better visualizer support for TestLoopV2
[Part 8](#11539): [test_loop]
Simple TestLoopEnvBuilder
github-merge-queue bot pushed a commit that referenced this pull request Jun 12, 2024
[test_loop] test_loop_sync_actor_maker implementation in TestLoopV2 (#…
c81ec74 
…11522)

Part 3

This PR has the implementation of sync_actor_maker function for test
loop.

We don't have control over when the sync_actor_maker function is called
during execution so we use an adhoc callback event to register the newly
created sync_actor with testloop.

### Other PRs

[Part 1](#11521): [test_loop]
Introduce TestLoopV2
[Part 2](#11520): [test_loop]
Change ClientQueries trait requirements in TestLoop util
[Part 3](#11522): [test_loop]
test_loop_sync_actor_maker implementation in TestLoopV2
[Part 4](#11523): [test_loop]
Introduce TestLoopPeerManagerActor for handling network messages across
clients
[Part 5](#11524): [test_loop]
Convert current tests to use TestLoopV2
[Part 6](#11525): [test_loop]
Cleanup old test loop code
[Part 7](#11528): [test_loop]
Better visualizer support for TestLoopV2
[Part 8](#11539): [test_loop]
Simple TestLoopEnvBuilder
github-merge-queue bot pushed a commit that referenced this pull request Jun 13, 2024
[test_loop] Introduce TestLoopPeerManagerActor for handling network m…
c77948d 
…essages across clients (#11523)

Part 4

This PR adds TestLoopPeerManagerActor for managing network messages.
Note that for handling messages between clients, the module needs to
have full details of all clients to route the message to.

The way TestLoopPeerManagerActor works is by registering a set of
handler functions that direct the messages to specific actors.

TestLoopPeerManagerActor comes with a set of default handlers, one for
client, partial_witness, and shards_manager. On top of these default
handlers, we have the ability to register override handlers which are
tried before the default handlers. The override handlers can do a bunch
of things like modify requests or drop requests.

Note that each client/node gets ONE SEPARATE implementation of
TestLoopPeerManagerActor. For a 4 client setup, we have 4 instances of
the peer manager actor. Each can have their own separate overrides.

### Other PRs

[Part 1](#11521): [test_loop]
Introduce TestLoopV2
[Part 2](#11520): [test_loop]
Change ClientQueries trait requirements in TestLoop util
[Part 3](#11522): [test_loop]
test_loop_sync_actor_maker implementation in TestLoopV2
[Part 4](#11523): [test_loop]
Introduce TestLoopPeerManagerActor for handling network messages across
clients
[Part 5](#11524): [test_loop]
Convert current tests to use TestLoopV2
[Part 6](#11525): [test_loop]
Cleanup old test loop code
[Part 7](#11528): [test_loop]
Better visualizer support for TestLoopV2
[Part 8](#11539): [test_loop]
Simple TestLoopEnvBuilder
github-merge-queue bot pushed a commit that referenced this pull request Jun 13, 2024
[test_loop] Convert current tests to use TestLoopV2 (#11524)
a9ca94b 
Part 5

🎉🎉🎉🎉🎉 

We can follow this up with a test loop builder to avoid duplicating
logic.

### Other PRs

[Part 1](#11521): [test_loop]
Introduce TestLoopV2
[Part 2](#11520): [test_loop]
Change ClientQueries trait requirements in TestLoop util
[Part 3](#11522): [test_loop]
test_loop_sync_actor_maker implementation in TestLoopV2
[Part 4](#11523): [test_loop]
Introduce TestLoopPeerManagerActor for handling network messages across
clients
[Part 5](#11524): [test_loop]
Convert current tests to use TestLoopV2
[Part 6](#11525): [test_loop]
Cleanup old test loop code
[Part 7](#11528): [test_loop]
Better visualizer support for TestLoopV2
[Part 8](#11539): [test_loop]
Simple TestLoopEnvBuilder
Base automatically changed from shreyan/testloop/pr5 to master June 13, 2024 01:18
@shreyan-gupta shreyan-gupta enabled auto-merge June 13, 2024 03:20
@shreyan-gupta shreyan-gupta added this pull request to the merge queue Jun 13, 2024
Merged via the queue into master with commit 80f8df6 Jun 13, 2024
27 of 29 checks passed
@shreyan-gupta shreyan-gupta deleted the shreyan/testloop/pr6 branch June 13, 2024 03:55
github-merge-queue bot pushed a commit that referenced this pull request Jun 13, 2024
[test_loop] Better visualizer support for TestLoopV2 (#11528)
9cbbeb8 
Part 7

Turns out for the visualizer tool to work, we require the "description"
emitted in `EventStartLogOutput` to be of a very specific format
`(node_id, Title(Subtitle(...)))`

In this PR, we try to add these additional details during logging. This
isn't as great as what we originally had as we don't pretty print the
individual event types etc. but still should be good enough for
debugging.

We can further improve this later as per necessity.

### Other PRs

[Part 1](#11521): [test_loop]
Introduce TestLoopV2
[Part 2](#11520): [test_loop]
Change ClientQueries trait requirements in TestLoop util
[Part 3](#11522): [test_loop]
test_loop_sync_actor_maker implementation in TestLoopV2
[Part 4](#11523): [test_loop]
Introduce TestLoopPeerManagerActor for handling network messages across
clients
[Part 5](#11524): [test_loop]
Convert current tests to use TestLoopV2
[Part 6](#11525): [test_loop]
Cleanup old test loop code
[Part 7](#11528): [test_loop]
Better visualizer support for TestLoopV2
[Part 8](#11539): [test_loop]
Simple TestLoopEnvBuilder
github-merge-queue bot pushed a commit that referenced this pull request Jun 13, 2024
[test_loop] Simple TestLoopEnvBuilder (#11539)
4fc5eab 
Part 8

Reduce code duplication by creating a simple test loop environment
builder.

Over time we can add more functionality to this as per requirements.

[Part 1](#11521): [test_loop]
Introduce TestLoopV2
[Part 2](#11520): [test_loop]
Change ClientQueries trait requirements in TestLoop util
[Part 3](#11522): [test_loop]
test_loop_sync_actor_maker implementation in TestLoopV2
[Part 4](#11523): [test_loop]
Introduce TestLoopPeerManagerActor for handling network messages across
clients
[Part 5](#11524): [test_loop]
Convert current tests to use TestLoopV2
[Part 6](#11525): [test_loop]
Cleanup old test loop code
[Part 7](#11528): [test_loop]
Better visualizer support for TestLoopV2
[Part 8](#11539): [test_loop]
Simple TestLoopEnvBuilder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pugachAG pugachAG pugachAG approved these changes

@tayfunelmas tayfunelmas Awaiting requested review from tayfunelmas

@Longarithm Longarithm Awaiting requested review from Longarithm

@staffik staffik Awaiting requested review from staffik

@robin-near robin-near Awaiting requested review from robin-near

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shreyan-gupta @pugachAG