Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 87 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ fn main() {
(
module: services::packet_processor::start_inbound,
name: inbound_packet_processor,
dependencies: [messenger, player_state, block_state, patchwork_state]
dependencies: [messenger, player_state, block_state, patchwork_state],
extras: [None]
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think extras is the right name for this. I don't know what the right name is because I'm not entirely sure what it's for, but if this is something we want to include in the services API we should give it a name that matches that particular use.

),
(
module: services::connection::start,
Expand Down Expand Up @@ -96,3 +97,88 @@ fn main() {
messenger.sender(),
);
}

#[cfg(test)]
mod tests {
use crate::*;

fn start_trace() {
let logger_config = ConfigBuilder::new()
.set_max_level(LevelFilter::Off)
.set_thread_level(LevelFilter::Off)
.set_target_level(LevelFilter::Off)
.build();
SimpleLogger::init(LevelFilter::Trace, logger_config).unwrap();
}

#[test]
fn test() {
start_trace();

// Since servers handle connection in their own thread, create a channel
// to retrieve information
let (router_sender, router_receiver) = std::sync::mpsc::channel();
let optional_router_sender = Some(router_sender.clone());

define_services!(
(
module: services::player::start,
name: player_state,
dependencies: [messenger]
),
(
module: services::block::start,
name: block_state,
dependencies: [messenger]
),
(
module: services::patchwork::start,
name: patchwork_state,
dependencies: [messenger, inbound_packet_processor, player_state]
),
(
module: services::messenger::start,
name: messenger,
dependencies: []
),
(
module: services::packet_processor::start_inbound,
name: inbound_packet_processor,
dependencies: [messenger, player_state, block_state, patchwork_state],
extras: [optional_router_sender]
),
(
module: services::connection::start,
name: connection_service,
dependencies: [messenger, player_state, patchwork_state, inbound_packet_processor]
),
(
module: services::keep_alive::start,
name: keep_alive,
dependencies: [messenger]
)
);
trace!("Services Started");
Copy link
Owner

Choose a reason for hiding this comment

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

Don't need this


// the stuff below this should also probably be moved to a service model
let peer_address = String::from("127.0.0.1");
let peer_port = std::env::var("PEER_PORT").unwrap().parse::<u16>().unwrap();
Comment on lines +163 to +165
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't have this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So then if I understand correctly the tests will be run using only cargo test, thus starting both servers in the same thread.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah definitely it should just be one call to cargo test. We shouldn't be using any networking in the test ideally


patchwork_state.sender().new_map(models::map::Peer {
port: peer_port,
address: peer_address,
});

std::thread::spawn(move || {
server::listen(
inbound_packet_processor.sender(),
connection_service.sender(),
messenger.sender(),
);
});

while let Ok((state, packet)) = router_receiver.recv() {
trace!("==[Received]== {:?}, {:?}", state, packet);
Copy link
Owner

Choose a reason for hiding this comment

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

I see you haven't put in any actual tests- what will be tested here once you do?

}
}
}
9 changes: 5 additions & 4 deletions src/services/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,16 @@ impl<O> ServiceInstance<O> {
// 1. Create the service instance struct (which creates a channel for you)
// 2. Run the service event loop method with a clone of the sender of all services it depends on
macro_rules! define_services {
($( (module: $service:path, name: $service_instance:ident, dependencies: [$($dependency:ident),*])),*) => (
($( (module: $service:path, name: $service_instance:ident, dependencies: [$($dependency:ident),*] $(, extras: [$($extra:ident),*])?)),*) => (
$(let mut $service_instance = ServiceInstance::new();)*
$(
paste::expr! {
$(let [<$dependency _clone>] = $dependency.sender(););*
$(let [<$dependency _clone>] = $dependency.sender();)*
$($(let [<$extra _clone>] = $extra.clone();)*)?
let sender = $service_instance.sender();
let receiver = $service_instance.receiver();
thread::spawn(move || $service(receiver, sender, $({[<$dependency _clone>]}),*));
thread::spawn(move || $service(receiver, sender $(, {[<$dependency _clone>]})* $(, $({[<$extra _clone>]})*)? ));
}
)*
)
);
}
12 changes: 11 additions & 1 deletion src/services/packet_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::interfaces::packet_processor::Operations;
use super::interfaces::patchwork::PatchworkState;
use super::interfaces::player::PlayerState;

use super::packet::{read, translate};
use super::packet::{read, translate, Packet};
use super::packet_handlers::packet_router;
use super::translation::{TranslationInfo, TranslationUpdates};
use std::collections::HashMap;
Expand All @@ -24,6 +24,7 @@ pub fn start_inbound<
player_state: P,
block_state: B,
patchwork_state: PA,
test_sender: Option<std::sync::mpsc::Sender<(i32, Packet)>>,
) {
let mut translation_data = HashMap::<Uuid, TranslationInfo>::new();

Expand All @@ -37,6 +38,15 @@ pub fn start_inbound<

let packet = read(&mut msg.cursor.clone(), translation_data.state);
let packet = translate(packet, translation_data.clone());

// Send raw packet info if we provided a channel
let test_sender_clone = test_sender.clone();
if let Some(test_sender_clone) = test_sender_clone {
test_sender_clone
.send((translation_data.state, packet.clone()))
.expect("Failed to send packet to channel");
}
Comment on lines +43 to +48
Copy link
Owner

Choose a reason for hiding this comment

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

What are we trying to accomplish with this? Are we trying to test our ability to read packets from raw data into structs?

Copy link
Collaborator Author

@Serdok Serdok Feb 13, 2020

Choose a reason for hiding this comment

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

My thought process was that to avoid lots of code duplication, we need a way to send data to the test thread to see the data. We cannot access the data that's inside the packets in the functions to see if its contents are what we expected.

Copy link
Owner

Choose a reason for hiding this comment

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

Why don't we just use the receiver this already has?


let translation_update = packet_router::route_packet(
packet,
translation_data.state,
Expand Down