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

Extract Network and Interface from utils #127

Merged
merged 18 commits into from
Feb 21, 2019
Merged

Extract Network and Interface from utils #127

merged 18 commits into from
Feb 21, 2019

Conversation

Hoverbear
Copy link
Contributor

@Hoverbear Hoverbear commented Sep 21, 2018

We would like to be able to use this code in benchmarks (#109) and other tests but currently there is no way to use Network or Interface from non-integration test code.

This introduces a harness feature which exposes harness::{Interface, Network}.

There is some rationale to how this was done:

  • We cannot have a module in tests/ that is built for benchmarking, so they were moved into a test harness module.
  • We cannot use #[cfg(test)] or #[cfg(debug_assertions)] to have this only build in test or development, since bench marking builds in release. So we have to make a feature flag.
  • We cannot control which features are used during cargo test or cargo bench by default without altering all defaults (such as what users get when they import Raft). So we must make this a default feature.

Benefits:

  • Opens the harness to users in their application code for exploring or testing.
  • Opens the harness to benchmarking and unit tests.
  • Allows Network and Interface to have their own modules as per Module shuffling and organizing #125 .

Detriments:

  • Users may get confused, but the documentation is quite explicit that it is simulator code.
  • It will slightly increase non cargo test compile time. The impact should be minimal.
  • Users may wish to disable the harness, which would require manual work on their end.

@Hoverbear Hoverbear added the Enhancement An improvement to existing code. label Sep 21, 2018
@Hoverbear Hoverbear added this to the 0.5.0 milestone Sep 21, 2018
@Hoverbear Hoverbear self-assigned this Sep 21, 2018
@hicqu
Copy link
Contributor

hicqu commented Oct 9, 2018

Make harness as a default feature is not very friendly for user. For example, TiKV needs to disable the feature when declar the raft dependency. So how about make harness as a sub crate?

@Hoverbear
Copy link
Contributor Author

@hicqu I think "need" is a very strong word, the only reason for TiKV to disable the feature is if we were worried a few milliseconds of compile time.

I think the idea of using a component is very interesting. I will explore this. :)

@Hoverbear
Copy link
Contributor Author

@hicqu Your idea was very good I think! PTAL again?

@hicqu
Copy link
Contributor

hicqu commented Nov 8, 2018

LGTM. Please fix the conflict. 👍

@Hoverbear
Copy link
Contributor Author

@hicqu Sorry for the delay. :)

@Hoverbear Hoverbear modified the milestones: 0.5.0, 0.6.0 Feb 11, 2019
src/lib.rs Outdated Show resolved Hide resolved
@Hoverbear
Copy link
Contributor Author

@BusyJay PTAL! :) Resolved.

@Hoverbear
Copy link
Contributor Author

PTAL @hicqu @AndreMouche :)

Signed-off-by: Ana Hobden <[email protected]>
Copy link
Member

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

Rest LGTM

harness/src/interface.rs Show resolved Hide resolved
harness/src/network.rs Show resolved Hide resolved
@Hoverbear
Copy link
Contributor Author

Merging with 2 LGTM from @hicqu and @AndreMouche (they said LGTM but seems it not approve it in the Github UI)

@Hoverbear Hoverbear merged commit a726ccc into master Feb 21, 2019
@Hoverbear Hoverbear deleted the test-reorganize branch February 21, 2019 19:25
This was referenced Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement An improvement to existing code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants