feat: delta api implementation #626
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
sjaanus
changed the title
sjaanus
changed the title
sjaanus
changed the title
server/tests/delta_test.rs
Outdated
|
|
||
| #[actix_web::test] | ||
| #[tracing_test::traced_test] | ||
| async fn test_delta() { |
There was a problem hiding this comment.
Testing whether, after the ETag is provided by Edge, Edge can successfully apply the delta when Unleash sends a new delta.
server/tests/delta_test.rs
Outdated
| @@ -0,0 +1,159 @@ | |||
| mod delta_test { | |||
There was a problem hiding this comment.
This is a weird place to put tests in my opinion. Are these very long running or a pain to setup for some reason? Can they not just put alongside the code that they're testing?
There was a problem hiding this comment.
Feature refresher is already so big, 1700 lines. It is super hard to follow as a newcomer.
There was a problem hiding this comment.
I think that's a signal that feature refresher is too big and new code needs to be split out, rather than moving tests out
There was a problem hiding this comment.
Probably add a new delta_refresher module, which can still extend FeatureRefresher with methods by using
impl FeatureRefresher {
/// The delta code
}
#[cfg(test)]
mod tests {
/// our delta tests, that way we won't have to compile a separate module either.
}That also means we can readd the #[cfg(test)] to the new method for unleash client.
There was a problem hiding this comment.
Doing it that way, also allows doing what Simon suggests and add a delta compiler flag
basically, in the http module, you'd declare
#[cfg(feature = "delta")]
mod delta_refresher;There was a problem hiding this comment.
But also a good observation on the size (I'm assuming the 1700 lines includes the test part of the code as well).
It might point towards feature_refresher needing to be a folder with multiple files rather than one file.
There was a problem hiding this comment.
Added it behind compiler flag.
| @@ -217,6 +217,10 @@ pub struct EdgeArgs { | |||
| #[clap(long, env, default_value_t = false, requires = "strict")] | |||
| pub streaming: bool, | |||
|
|
|||
| /// If set to true. Edge connects to upstream using delta polling instead of normal polling. This is experimental feature and might and change. | |||
There was a problem hiding this comment.
| /// If set to true. Edge connects to upstream using delta polling instead of normal polling. This is experimental feature and might and change. | |
| /// If set to true. Edge connects to upstream using delta polling instead of normal polling. This is experimental feature and might change. |
| } | ||
|
|
||
|
|
||
| #[cfg(feature = "delta")] |
There was a problem hiding this comment.
Since you're always adding the impl part, I'm not sure if there's much value in using a feature flag just for tests.
| use crate::http::refresher::feature_refresher::FeatureRefresher; | ||
| use crate::tokens::cache_key; | ||
|
|
||
| impl FeatureRefresher { |
There was a problem hiding this comment.
Either add #[cfg(feature = "delta")] here, or remove it from the tests part.
| @@ -210,7 +218,6 @@ impl UnleashClient { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
There was a problem hiding this comment.
Readding this one seems useful.
Now edge can understand and talk to Unleash Delta API. There is cli flag delta to enable this feature.