Skip to content

Tests skeleton#146

Open
Serdok wants to merge 6 commits intoDuncanUszkay1:masterfrom
Serdok:master
Open

Tests skeleton#146
Serdok wants to merge 6 commits intoDuncanUszkay1:masterfrom
Serdok:master

Conversation

@Serdok
Copy link
Collaborator

@Serdok Serdok commented Feb 12, 2020

Tests skeleton

Issue: Integration tests (Issue 127)

Why

We are at a point of the development where we need to automate tests. The main advantage of tests over the manual integration testing would be that we can check correctness on our codes quicker (no need to launch a Minecraft client for the moment, we can manually send packages)

What

The purpose of this PR is to show how tests may be organized. We control packets with a new channel provided in unit tests. This allows us to automatically receive any packets that go through a service and manually send a packet to see how said service interact with it.
To expand these tests, we need to add parameters to the service starters and provide them using the macro in the extras field.
The difference between this PR and the previous one is that there is much less code duplication, and the only thing we need to add to test a service is extra parameters to the starter functions

Checks

  • [ x ] I have performed the manual integration test
  • [ x ] The output of cargo clippy is empty
  • [ x ] I have run cargo fmt on my most recent changes
  • [] I haven't read the PR template

Comment on lines +43 to +48
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");
}
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?

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.

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

Comment on lines +163 to +165
// 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();
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

});

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?

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