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

UTs for service functionalities #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VaralakshmiKA
Copy link
Contributor

Signed-off-by: Vlakshmi [email protected]

Copy link
Collaborator

@rranjan3 rranjan3 left a comment

Choose a reason for hiding this comment

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

Log crates might not need to be brought in scope again.

@@ -61,7 +127,6 @@ impl Poet2Service {
}

pub fn get_block(&mut self, block_id: BlockId) -> Block {
debug!("Getting block {:?}", block_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should this log be removed?

Choose a reason for hiding this comment

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

The block_id's value is unknown as it's not holding any value,so it should be removed.

@@ -84,15 +149,13 @@ impl Poet2Service {
sleep(time::Duration::from_secs(1));
summary = self.service.summarize_block();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ensure no changes apart from the intended are made to the codebase.

use log::LogLevelFilter;
use log4rs::append::console::ConsoleAppender;
use log4rs::config::{Appender, Config, Root};
use log4rs::encode::pattern::PatternEncoder;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are log crates needed to be brought in scope here ?

test_mock_str.insert(String::from("test_min"), String::from("10"));
debug!("In Mocking get block");
let block_id_map = BlockId::from(vec![16,16,85,32]);
let block_id_test = BlockId::from(vec![16,16,85,32]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does having repetitive block ids(16) here serve any purpose?

Choose a reason for hiding this comment

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

I don't think so as it's a map so key should be unique.

if wt > min_time && wt < max_time{
assert_gt!(wt, min_time);
assert_lt!(wt, max_time);
debug!("wait_time is generated within the range {:?} and {:?}, {:?}",min_time, max_time, wt);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need not have log here.

cal_time.get_settings.return_value(test_result);
let mut svc = Poet2Service::new( Box::new(cal_time));
let mut wt = svc.calculate_wait_time(Default::default()).as_secs();
debug!("Wait time generated should be zero and wait_time is {:?}", wt);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Log not needed here. Assert might have an option to add some description.

@@ -323,7 +505,6 @@ mod tests {
svc.broadcast(Default::default());
assert_eq!(2, 2);
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this commented piece of code as well

cal_time.get_settings.return_value(test_result);
let mut svc = Poet2Service::new( Box::new(cal_time));
let mut wt = svc.calculate_wait_time(Default::default()).as_secs();
assert_eq!(wt, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be 0 or some default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anything is fine.As default value is not declared in tests scope, made it to 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try having some other non-zero default value and then assert it.

Copy link
Collaborator

@rranjan3 rranjan3 left a comment

Choose a reason for hiding this comment

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

There are yet other comments you need to address. Also merge it into a single commit.
You still do not have a descriptive comment for your commit.

Signed-off-by: Vlakshmi <[email protected]>

Made changes as suggested

Signed-off-by: Vlakshmi <[email protected]>

Fixed comments raised

Signed-off-by: Vlakshmi <[email protected]>

removed comments in the code

Signed-off-by: Vlakshmi <[email protected]>
@@ -12,3 +12,5 @@ protobuf="2.0"
zmq = { git = "https://github.com/erickt/rust-zmq", branch = "release/v0.8" }
num = "*"
clap = "~2.27.0"
double = "0.2.2"
Copy link
Owner

Choose a reason for hiding this comment

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

why do we need the double crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

double is the mocking library we are using here.

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.

4 participants