Refactor: move shared test functionality to a dev-dependency #170

mickvandijke · 2023-02-02T15:20:37Z

You can't share test functionality between unit tests or unit tests <-> integration tests, without defining this functionality in production code. Creating a "test-helpers" dev-dependency will solve that.

This PR solves:

Sharing functionality between tests defined in different modules.
Separating test only code from production.

Prerequisites

Sometimes we want to call production code from the test-helpers dependency. This will however create a cyclic dependency issue with our current code (test-helpers <-> torrust-tracker). Instead of these two crates depending on each other, we should instead make them dependent on the same sub-crates. So we must isolate code in their own crates as much as possible.

For example the tracker configuration code. This can be easily moved to its own crate. Then we would be able to import it using the test-helpers crate and the torrust-tracker crate without creating a cyclic dependency issue.

Isolate configuration and primitives in their own crates #174

mickvandijke · 2023-02-16T16:05:03Z

Hi @josecelano ,

Do you know why my latest commit fails the cargo fmt --check command in the pipeline, despite it not failing when running it locally?

josecelano · 2023-02-16T16:26:57Z

Hi @josecelano ,

Do you know why my latest commit fails the cargo fmt --check command in the pipeline, despite it not failing when running it locally?

Hi @WarmBeer, you can check in on the workflow output.

I have a problem I have not been able to fix. For some reason, when the Visual Studio Code changes a mod.rs file, the linter is not executed, and it does not fail in my local version either. If I save the file manually, then the error is fixed. Normally the problem is that the pub mod whatever lines are not sorted alphabetically. Make sure you have the same rustc --version, and you execute the same command cargo clippy --all-targets -- -D clippy::pedantic. In my case, even if I run that command manually, I do not get the error.

Try to run the linter locally for the file that has the error in the pipeline and using exactly the same configuration. We have to find out what this is happening. It's a pretty annoying error, especially when you realize there was an error in the pipeline some commits later :-(.

mickvandijke · 2023-02-20T12:28:52Z

cargo clippy --all-targets -- -D clippy::pedantic

Thank you @josecelano !

I was using cargo fmt --check instead of cargo clippy --all-targets -- -D clippy::pedantic. Now I see some errors, although they are different from the errors in the pipeline :'). I am using rustc 1.67.1, same as the pipeline.

mickvandijke · 2023-02-21T15:11:40Z

cargo clippy --all-targets -- -D clippy::pedantic

Thank you @josecelano !

I was using cargo fmt --check instead of cargo clippy --all-targets -- -D clippy::pedantic. Now I see some errors, although they are different from the errors in the pipeline :'). I am using rustc 1.67.1, same as the pipeline.

Hey @josecelano ,

I think the issue lies here: https://github.com/torrust/torrust-tracker/actions/runs/4233786546/workflow#L16 . The format job in the pipeline runs on the nightly toolchain instead of the default stable.

When I run cargo +nightly fmt --check, I get the same results as the pipeline. :)

I do prefer the nightly fmt though, so I guess we should keep it like this.

josecelano · 2023-02-21T16:45:34Z

cargo clippy --all-targets -- -D clippy::pedantic

Thank you @josecelano !
I was using cargo fmt --check instead of cargo clippy --all-targets -- -D clippy::pedantic. Now I see some errors, although they are different from the errors in the pipeline :'). I am using rustc 1.67.1, same as the pipeline.

Hey @josecelano ,

I think the issue lies here: https://github.com/torrust/torrust-tracker/actions/runs/4233786546/workflow#L16 . The format job in the pipeline runs on the nightly toolchain instead of the default stable.

When I run cargo +nightly fmt --check, I get the same results as the pipeline. :)

I do prefer the nightly fmt though, so I guess we should keep it like this.

Upss, sorry, I thought I told you I was also using the nightly:

$ rustc --version
rustc 1.69.0-nightly (d7948c843 2023-01-26)

I think I started using it when @da2ce7 was working on fixing all clipply warnings.

mickvandijke · 2023-02-22T17:38:24Z

Hi @josecelano , @da2ce7 ,

This PR is ready for review :).

da2ce7 · 2023-02-22T23:30:08Z

I have made a squashed version of this pull request, for easier review: #193 I think that @WarmBeer could consider squashing these commits. (Or can git reset --hard da2ce7/156-warm_beer, for this branch).

josecelano · 2023-03-01T15:56:29Z

src/http/tracker_interface.rs

+
+/// Trait to be implemented by a http interface for the tracker.
+#[allow(clippy::module_name_repetitions)]
+pub trait TrackerInterfaceTrait: Sync + Send {


hi @WarmBeer, I usually try to avoid using words like "Interface", "Trait", "Base" in class/interface/struct names. Besides, with that name, it seems you are implementing a Tracker. What do you think about HttpTrackerStarter or HttpTrackerLauncher?

I see you have the other struct:

pub struct TrackerInterface<S> { cfg: torrust_tracker_configuration::HttpTracker, state: S, }

Maybe we could call this one just HttpTrackerHandler or HttpServerHandler, depending on whether we want to use HttpServer or HttpTrackerServer for the thread running the service.

In fact, you are already using those names in:

pub type StoppedHttpServer = TrackerInterface<Stopped>; pub type RunningHttpServer = TrackerInterface<Running>;

Why not this?:

pub type StoppedHttpServer = HttpServer<Stopped>; pub type RunningHttpServer = HttpServer<Running>;

In summary, you would have:

pub trait HttpServerStarter: Sync + Send { fn start_with_graceful_shutdown<F>(...) -> ... } pub struct HttpServer<S> { cfg: torrust_tracker_configuration::HttpTracker, state: S, } pub type StoppedHttpServer = HttpServer<Stopped>; pub type RunningHttpServer = HttpServer<Running>; impl<I: HttpServerStarter + 'static> StoppedHttpServer { ... } impl<I: HttpServerStarter> RunningHttpServer { ... }

Hi @josecelano ,

To provide a bit of background to my commits:

We have talked about making the tracker agnostic of the server or interface that is being used to access it, so that we can run it using any http or udp server. As of right now, we already have two http server implementations: Axum and Warp.

To not have to deal with a separate server struct for every implementation when starting them, I have introduced the TrackerInterface wrapper struct. This TrackerInterface struct wraps any http server implementation that implements the trait TrackerInterfaceTrait. Like axum and warp.

The reason that I chose the word interface is because to me the Tracker struct is the core of our tracker. And udp and http are just interfaces to interact with the public functions of our core Tracker. But I see that the name might be too confusing.

I think that naming the types like this would avoid any confusion:

struct http::TrackerInterface -> http::HttpServerWrapper
trait http::TrackerInterfaceTrait -> http::HttpServer

What do you think?

Actually after thinking about it I like the name “HttpServerStarter” :)

As the struct that implements the trait is only a launcher of the a http server and not the entire server itself.

hi, @WarmBeer thank you for clarifying it!

First at all, We are going to remove the Warp implementation soon, but I guess we could have different tracker versions in the future: V1, V2, etcetera. But in that case, I think it would not work because we would break that "interface".

Secondly, I agree that the Tracker is the "core" tracker, and the UDP and HTTP trackers are wrappers, but I think they are not only thin wrappers. From my perspective, the Tracker is a domain service and the UDPTracker, and
HTTPTracker are application services.

I think UDP and HTTP tracker services have these main responsibilities:

A different "delivery" layer.

Authentication

Collect specific statistics

https://enterprisecraftsmanship.com/posts/domain-vs-application-services/

I think the API would be another App service as well.

Finally, I usually call "server" a running service. In our case, the thread which is running the app service (UDP, HTTP or API).

What do you think?

On the other hand, I find it strange that the core Tracker handles HTTP authentication for the HTTP tracker.

In the end, I suppose it depends on how you see it, but I think the current core Tracker already has too many responsibilities. I would even more functions from the tracker (I think you have some comments about it, for example for persistence).

Actually after thinking about it I like the name “HttpServerStarter” :)

As the struct that implements the trait is only a launcher of the a http server and not the entire server itself.

I think its only responsibilities are:

Launching the actual server

Knowing its state

Knowing the configuration needed to launch it. Secondary responsibility derived from the first one.

josecelano · 2023-03-01T16:28:00Z

tests/common/tracker.rs

+}
+
+// TODO: Move to test-helpers crate once `Tracker` is isolated.
+pub fn tracker_instance(configuration: &Arc<torrust_tracker_configuration::Configuration>) -> Arc<Tracker> {


hi @WarmBeer tracker_isntance makes me think that's a singleton and not a factory. Maybe tracker_factory or new_tracker_instance. I would choose the first one because it's not a constructor; it's a dependency "resolver". In fact, I think this is the app container and bootstrapping at the same time.

josecelano · 2023-03-01T16:32:22Z

src/apis/server.rs


    use crate::apis::server::ApiServer;
    use crate::tracker;
    use crate::tracker::statistics;

    fn tracker_configuration() -> Arc<Configuration> {
-        Arc::new(ephemeral())
+        Arc::new(torrust_tracker_test_helpers::configuration::ephemeral())


I think it would be enougth with:

Arc::new(configuration::ephemeral())

josecelano

Cool!

josecelano · 2023-03-02T19:07:17Z

Hi @WarmBeer I think we have a similar pattern here when we run the stats event listener:

impl Keeper {
    #[must_use]
    pub fn new() -> Self {
        Self { repository: Repo::new() }
    }

    #[must_use]
    pub fn new_active_instance() -> (Box<dyn EventSender>, Repo) {
        let mut stats_tracker = Self::new();

        let stats_event_sender = stats_tracker.run_event_listener();

        (stats_event_sender, stats_tracker.repository)
    }

    pub fn run_event_listener(&mut self) -> Box<dyn EventSender> {
        let (sender, receiver) = mpsc::channel::<Event>(CHANNEL_BUFFER_SIZE);

        let stats_repository = self.repository.clone();

        tokio::spawn(async move { event_listener(receiver, stats_repository).await });

        Box::new(Sender { sender })
    }
}

I've added a new test for the tracker and I'm getting this error:

thread 'tracker::tests::the_tracker::configured_as_whitelisted::handling_an_scrape_request::it_should_be_able_to_build_a_zeroed_scrape_data_for_a_list_of_info_hashes' panicked at 'there is no reactor running, must be called from the context of a Tokio 1.x runtime', src/tracker/statistics.rs:74:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It's seems it needs that listeners to be ready but it's strange because I'm testing a simple method (which is not async) and does not have any dependency. Anyway, it's only related to this PR but I suppose we should apply the same pattern (state machine) you have applied here. We can do it in a new PR.

mickvandijke · 2023-03-02T22:01:35Z

Hi @WarmBeer I think we have a similar pattern here when we run the stats event listener:

impl Keeper {
    #[must_use]
    pub fn new() -> Self {
        Self { repository: Repo::new() }
    }

    #[must_use]
    pub fn new_active_instance() -> (Box<dyn EventSender>, Repo) {
        let mut stats_tracker = Self::new();

        let stats_event_sender = stats_tracker.run_event_listener();

        (stats_event_sender, stats_tracker.repository)
    }

    pub fn run_event_listener(&mut self) -> Box<dyn EventSender> {
        let (sender, receiver) = mpsc::channel::<Event>(CHANNEL_BUFFER_SIZE);

        let stats_repository = self.repository.clone();

        tokio::spawn(async move { event_listener(receiver, stats_repository).await });

        Box::new(Sender { sender })
    }
}

I've added a new test for the tracker and I'm getting this error:

thread 'tracker::tests::the_tracker::configured_as_whitelisted::handling_an_scrape_request::it_should_be_able_to_build_a_zeroed_scrape_data_for_a_list_of_info_hashes' panicked at 'there is no reactor running, must be called from the context of a Tokio 1.x runtime', src/tracker/statistics.rs:74:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It's seems it needs that listeners to be ready but it's strange because I'm testing a simple method (which is not async) and does not have any dependency. Anyway, it's only related to this PR but I suppose we should apply the same pattern (state machine) you have applied here. We can do it in a new PR.

Hi @josecelano ,

You can only call tokio::spawn from a tokio runtime. Even though you can call this function in a non async way, you'll still have to add #[tokio::test] to the parent test function.

mickvandijke · 2023-03-02T22:05:29Z

Hi @josecelano ,

I have fixed most tests that I broke with my latest commit. There are however still 18 tests that fail and I don't really know why. It seems that most of these fails have something to do with this error I get from the tracker:

"d14:failure reason109:cannot get the client IP from the connection info in src/http/axum_implementation/extractors/peer_ip.rs:50:23e"

Do you know what causes this?

josecelano · 2023-03-02T23:09:37Z

Hi @josecelano ,

I have fixed most tests that I broke with my latest commit. There are however still 18 tests that fail and I don't really know why. It seems that most of these fails have something to do with this error I get from the tracker:

"d14:failure reason109:cannot get the client IP from the connection info in src/http/axum_implementation/extractors/peer_ip.rs:50:23e"

Do you know what causes this?

Hi @WarmBeer I'm using this crate

https://docs.rs/axum-client-ip/latest/axum_client_ip/

To resolve the remote client IP and it requires to start the server like this:

.serve(
// Don't forget to add `ConnetInfo` if you aren't behind a proxy
 app.into_make_service_with_connect_info::<SocketAddr>() )

I think you probably remove that part, but I haven't checked it.

mickvandijke · 2023-03-02T23:14:35Z

Hi @josecelano ,
I have fixed most tests that I broke with my latest commit. There are however still 18 tests that fail and I don't really know why. It seems that most of these fails have something to do with this error I get from the tracker:
"d14:failure reason109:cannot get the client IP from the connection info in src/http/axum_implementation/extractors/peer_ip.rs:50:23e"
Do you know what causes this?

Hi @WarmBeer I'm using this crate

https://docs.rs/axum-client-ip/latest/axum_client_ip/

To resolve the remote client IP and it requires to start the server like this:
.serve(
// Don't forget to add `ConnetInfo` if you aren't behind a proxy
 app.into_make_service_with_connect_info::<SocketAddr>() )
I think you probably remove that part, but I haven't checked it.

Thank you @josecelano , that fixed it!

josecelano · 2023-03-03T08:04:01Z

src/http/tracker_interface.rs

@@ -0,0 +1,100 @@
+use std::future::Future;


Hi @WarmBeer should we also rename the mod tracker_interface?

Yes, I forgot :)

josecelano · 2023-03-03T08:05:58Z

src/tracker/services/torrent.rs

        use crate::protocol::info_hash::InfoHash;
        use crate::tracker::services::common::tracker_factory;
        use crate::tracker::services::torrent::tests::sample_peer;
        use crate::tracker::services::torrent::{get_torrent_info, Info};

        pub fn tracker_configuration() -> Arc<Configuration> {
-            Arc::new(ephemeral_configuration())
+            Arc::new(torrust_tracker_test_helpers::configuration::ephemeral())


I've commented on another line like this:

I think this should be enough:

Arc::new(configuration::ephemeral())

josecelano · 2023-03-03T08:17:25Z

src/apis/server.rs

+        let job = tokio::spawn(async move {
+            if let (true, Some(ssl_cert_path), Some(ssl_key_path)) = (cfg.ssl_enabled, cfg.ssl_cert_path, cfg.ssl_key_path) {
+                let tls_config = RustlsConfig::from_pem_file(ssl_cert_path, ssl_key_path)
+                    .await
+                    .expect("Could not read ssl cert and/or key.");
+
+                start_tls_from_tcp_listener_with_graceful_shutdown(listener, tls_config, &tracker, receiver)
+                    .await
+                    .expect("Could not start from tcp listener with tls.");
+            } else {
+                start_from_tcp_listener_with_graceful_shutdown(listener, &tracker, receiver)
+                    .await
+                    .expect("Could not start from tcp listener.");
+            }
+        });


Hi @WarmBeer,

This is related to this issue.

Has the behaviour changed when you cannot start the server with TSL?

Does the expect means the whole application is going to halt when the TLS configuration is missing? If that's true, should we just warn the user and try to start other services?

Hi @josecelano ,

Your question has kept me occupied for an hour xd. So the problem with these tokio spawns is that they will not abort the parent program when a panic occurs. So we won't actually know if there was a panic in a task.

You solved this partially here by using a channel to report whether the api server has started. The problem with our current code however is that it will only check whether the RusttlsConfig was created successfully and not if the actual server has started successfully.

We could add another channel that will report something when the await on the server start function has yielded an error. This way we can wait a little bit after starting the server start job/tokio task to see whether an error has been sent through this channel.

So to answer your questions:

Has the behaviour changed when you cannot start the server with TSL?

Yes and I need to fix it :).

Does the expect means the whole application is going to halt when the TLS configuration is missing? If that's true, should not we just warn the user and try to start other services?

Since the expect will yield a panic on a tokio task, only the task will abort, but the rest of the app will continue like nothing happened.

In my opinion, the entire app should abort when a panic occurs in any of the tasks.

Hi @WarmBeer, thank you for the clarification!

I think we should start all or nothing too. People don't read logs :-). If something fails and you don't care, you should explicitly disable it.

In summary, we need to change even the previous behaviour if it only shows a warning.

This is getting more and more complex. I do not know if we should add a second one-shot channel or maybe a normal channel and define a list of messages between the launcher and the "launched" thing. Alternatively, we could try to check if the service is alive and ready just using the public contract. For example, for the API, we could check if the status endpoint is returning a 200, but I do not like this solution because the launcher uses a "polling" approach. I think a proper communication channel between the laucher and the child is the right solution.

@josecelano

I will restore the current "develop" behaviour in this PR and will open a new issue for how we are going to deal with this.

OK @WarmBeer. I think we could replace the warning with an error, at least.

josecelano · 2023-03-07T18:26:38Z

tests/udp/test_environment.rs

+impl<S> TestEnvironment<S> {
+    /// Add a torrent to the tracker
+    #[allow(dead_code)]
+    pub async fn add_torrent(&self, info_hash: &InfoHash, peer: &Peer) {
+        self.tracker.update_torrent_with_peer_and_get_stats(info_hash, peer).await;
+    }
+}


Hi @WarmBeer, this is OK but if you read this discussion I think we should stop using that method. In fact, it should be private.

Besides, now we have a high-level public method to announce a new peer: Tracker::announce. Maybe in this case we could use:

impl<S> TestEnvironment<S> { /// Add a torrent to the tracker #[allow(dead_code)] pub async fn announce_torrent(&self, info_hash: &InfoHash, peer: &mut Peer, remote_client_ip: &IpAddr) { self.tracker.announce(info_hash, peer: &mut Peer, remote_client_ip).await; } }

Althougth that could have side effects we do not want in these tests.

josecelano · 2023-03-08T18:27:46Z

Hi @WarmBeer, I'm almost finished with #160. I expect to remove the HTTP tracker Warp implementation tomorrow. Maybe it's easier to resolve these conflicts after removing the Warp code.

mickvandijke · 2023-03-09T10:40:26Z

Hi @josecelano , @da2ce7 ,

This PR has been rebased and is ready for review/merging (finally).

josecelano

Hey @WarmBeer I think there are a lot of regressions. I think you reverted some changes that were already merged.

josecelano · 2023-03-09T10:47:35Z

src/apis/handlers.rs

@@ -17,7 +17,7 @@ use crate::apis::resources::auth_key::AuthKey;
 use crate::apis::resources::stats::Stats;
 use crate::apis::resources::torrent::ListItem;
 use crate::protocol::info_hash::InfoHash;
-use crate::tracker::auth::Key;
+use crate::tracker::auth::KeyId;


Hi @WarmBeer I renamed the KeyId to Key.

josecelano · 2023-03-09T10:49:33Z

src/apis/handlers.rs

@@ -86,7 +86,7 @@ pub async fn remove_torrent_from_whitelist_handler(
 }

 pub async fn reload_whitelist_handler(State(tracker): State<Arc<Tracker>>) -> Response {
-    match tracker.load_whitelist_from_database().await {
+    match tracker.load_whitelist().await {


Hi @WarmBeer I renamed the load_whitelist to load_whitelist_from_database because some of them had the suffix from_database and others don't:

load_keys_from_database

load_whitelist_from_database

load_torrents_from_database

josecelano · 2023-03-09T10:50:14Z

src/apis/resources/auth_key.rs

 }

-impl From<AuthKey> for auth::ExpiringKey {
+impl From<AuthKey> for auth::Key {


Hi @WarmBeer Key was reanamed to ExpiringKey

josecelano · 2023-03-09T10:51:17Z

src/apis/resources/auth_key.rs

+            auth::Key::from(auth_key_resource),
+            auth::Key {
+                key: "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM".to_string(), // cspell:disable-line
+                valid_until: Some(Current::add(&Duration::new(duration_in_secs, 0)).unwrap())


@WarmBeer , the valid_until attribute is not optional anymore.

mickvandijke · 2023-03-09T10:57:27Z

Hey @WarmBeer I think there are a lot of regressions. I think you reverted some changes that were already merged.

Hi @josecelano ,

Oops, that was not my intention. I did the same rebase steps as last time:

git clean -fx
git fetch —all
git switch develop
git merge --ff-only
git switch 156-fix-random-test-failure-due-to-port-collision
git reset develop
git add -A 
git commit -m "isolated configuration, test-helpers and primitives crates"
git push --force

Should I do a merge commit instead?

josecelano · 2023-03-09T11:34:17Z

Hey @WarmBeer I think there are a lot of regressions. I think you reverted some changes that were already merged.

Hi @josecelano ,

Oops, that was not my intention. I did the same rebase steps as last time:
git clean -fx
git fetch —all
git switch develop
git merge --ff-only
git switch 156-fix-random-test-failure-due-to-port-collision
git reset develop
git add -A 
git commit -m "isolated configuration, test-helpers and primitives crates"
git push --force
Should I do a merge commit instead?

Hi @WarmBeer I think that only works if 156-fix-random-test-failure-due-to-port-collision is based on develop, but there are some conflicts to resolve. I guess you should have rebased 156-fix-random-test-failure-due-to-port-collision onto develop, resolve the conflicts, and then apply the git reset develop to rewrite commits. It seems you added your changes but also removed all new changes in develop. I think the result is you do not have any of the latest changes since you started working on the new branch.

I would reset 156-fix-random-test-failure-due-to-port-collision to the commit before the reset and then repeat the process with:

git clean -fx
git fetch —all
git switch develop
git merge --ff-only
git switch 156-fix-random-test-failure-due-to-port-collision
git rebase develop
# Resolve conflicts

# Rewrite history
git reset develop
git add -A 
git commit -m "isolated configuration, test-helpers and primitives crates"
git push --force

…nd isolated crates

mickvandijke · 2023-03-09T13:26:08Z

Hi @josecelano , @da2ce7 ,

This should be ready for merging now :)

josecelano

Hi @WarmBeer I think all the Axum integration tests are missing. They were mostly the same but there are some new:

https://github.com/torrust/torrust-tracker/blob/develop/tests/http_tracker.rs

mickvandijke · 2023-03-09T15:18:43Z

Hey @josecelano ,

Whenever I try to merge origin/develop into this branch, I get wildly different files compared to origin/develop. It merges some functions where I have no idea where they came from. Some functions don't even come from either branch. For example:

tests/http_tracker.rs

mod axum_http_tracker_server {

    // WIP: migration HTTP from Warp to Axum

    use local_ip_address::local_ip;
    use torrust_tracker::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp;
    use torrust_tracker::http::axum_implementation::resources::ok::Ok;
    use torrust_tracker::http::Version;

    use crate::http::client::Client;
    use crate::http::server::start_default_http_tracker;

    #[tokio::test]
    async fn should_return_the_status() {
        // This is a temporary test to test the new Axum HTTP tracker server scaffolding

        let http_tracker_server = start_default_http_tracker(Version::Axum).await;

        let client_ip = local_ip().unwrap();

        let response = Client::bind(http_tracker_server.get_connection_info(), client_ip)
            .get("status")
            .await;

        let ok: Ok = serde_json::from_str(&response.text().await.unwrap()).unwrap();

        assert_eq!(
            ok,
            Ok {
                remote_client_ip: RemoteClientIp {
                    right_most_x_forwarded_for: None,
                    connection_info_ip: Some(client_ip)
                }
            }
        );
    }
...
}

This function does not even exist in origin/develop or 156-fix-random-test-failure-due-to-port-collision. Where does it come from?! I feel like I'm losing my mind. :')

josecelano · 2023-03-09T15:45:05Z

Hey @josecelano ,

Whenever I try to merge origin/develop into this branch, I get wildly different files compared to origin/develop. It merges some functions where I have no idea where they came from. Some functions don't even come from either branch. For example:

tests/http_tracker.rs

mod axum_http_tracker_server {

    // WIP: migration HTTP from Warp to Axum

    use local_ip_address::local_ip;
    use torrust_tracker::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp;
    use torrust_tracker::http::axum_implementation::resources::ok::Ok;
    use torrust_tracker::http::Version;

    use crate::http::client::Client;
    use crate::http::server::start_default_http_tracker;

    #[tokio::test]
    async fn should_return_the_status() {
        // This is a temporary test to test the new Axum HTTP tracker server scaffolding

        let http_tracker_server = start_default_http_tracker(Version::Axum).await;

        let client_ip = local_ip().unwrap();

        let response = Client::bind(http_tracker_server.get_connection_info(), client_ip)
            .get("status")
            .await;

        let ok: Ok = serde_json::from_str(&response.text().await.unwrap()).unwrap();

        assert_eq!(
            ok,
            Ok {
                remote_client_ip: RemoteClientIp {
                    right_most_x_forwarded_for: None,
                    connection_info_ip: Some(client_ip)
                }
            }
        );
    }
...
}

This function does not even exist in origin/develop or 156-fix-random-test-failure-due-to-port-collision. Where does it come from?! I feel like I'm losing my mind. :')

That was removed here.

It was added, but them removed later.

josecelano · 2023-03-09T18:30:39Z

@WarmBeer I've fixed conflicts and created a new PR.

@WarmBeer

4ad9815 refactor: remove duplicate code (Jose Celano) 0097c85 fix: clippy warnings (Jose Celano) cf9e9a9 fix: merge conflicts (Jose Celano) a611fbd chore: fix clippy errors (Warm Beer) d020c5a refactor: `tracker_api` launching and testing (Warm Beer) 5b95b5d refactor: renamed `tracker_interface` to `server` and shortened `configuration::ephemeral` calls (Warm Beer) fac2be8 fix: all http tracker tests (Warm Beer) d914e5c refactor: replaced test http servers with the new test environments (Warm Beer) 4f2b035 refactor: renamed `TrackerInterface` struct and relevant trait (Warm Beer) f40e43f refactor: prepend paths to ephemeral calls (Warm Beer) 191fbac refactor: moved signals mod to own file (Warm Beer) 504bad3 refactor: abstract away http server implementations (Warm Beer) a72c123 refactor: removed duplicate ephemeral configuration fn (Warm Beer) 164f29a isolated configuration, test-helpers and primitives crates (Warm Beer) Pull request description: Hi @WarmBeer, I've rebased your [PR](#170), fixing all merge conflicts. Do you want to rewrite the history of your changes or can we merge it as it's is? Now you can rewrite your commits if you want to, with the following: ```s git reset develop git add -A git commit -m "isolated configuration, test-helpers and primitives crates" git push --force ``` As you mentioned [here](#170 (comment)). ACKs for top commit: josecelano: ACK 4ad9815 Tree-SHA512: 64083f697bb24d2c0037c332555f6eaad46adf675f937f473ce0e2bd874a49e1d3581f4aecc73a131f5db30456248b369a0e1e1ef1f35b4024b131adfe557994

mickvandijke linked an issue Feb 2, 2023 that may be closed by this pull request

Fix random test failure due to port collision #156

Closed

josecelano mentioned this pull request Feb 2, 2023

Fix random test failure due to port collision #156

Closed

mickvandijke mentioned this pull request Feb 2, 2023

Isolate configuration and primitives in their own crates #173

Closed

mickvandijke added Code Cleanup / Refactoring Tidying and Making Neat testing labels Feb 2, 2023

da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Feb 4, 2023

mickvandijke marked this pull request as ready for review February 22, 2023 17:37

josecelano self-requested a review February 22, 2023 18:37

da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label Feb 22, 2023

da2ce7 self-requested a review February 22, 2023 23:16

da2ce7 mentioned this pull request Feb 22, 2023

refactor: move shared test functionality to a dev-dependency #193

Closed

mickvandijke force-pushed the 156-fix-random-test-failure-due-to-port-collision branch from ffc1307 to ce856bd Compare February 23, 2023 15:05

josecelano approved these changes Mar 1, 2023

View reviewed changes

josecelano approved these changes Mar 2, 2023

View reviewed changes

da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Mar 2, 2023

josecelano approved these changes Mar 3, 2023

View reviewed changes

josecelano mentioned this pull request Mar 3, 2023

Warn when API cannot be started on HTTPs #161

Closed

josecelano approved these changes Mar 3, 2023

View reviewed changes

mickvandijke mentioned this pull request Mar 3, 2023

Application should abort on panic in any of the threads #213

Closed

josecelano mentioned this pull request Mar 3, 2023

Add unit tests for Tracker #210

Merged

josecelano approved these changes Mar 7, 2023

View reviewed changes

mickvandijke force-pushed the 156-fix-random-test-failure-due-to-port-collision branch from efe78c0 to 46d545d Compare March 9, 2023 10:38

josecelano requested changes Mar 9, 2023

View reviewed changes

mickvandijke force-pushed the 156-fix-random-test-failure-due-to-port-collision branch from 46d545d to efe78c0 Compare March 9, 2023 12:43

fix(#156): added test environments for tests, server state machines a…

7e550c6

…nd isolated crates

mickvandijke force-pushed the 156-fix-random-test-failure-due-to-port-collision branch from 51d858b to 7e550c6 Compare March 9, 2023 13:23

josecelano requested changes Mar 9, 2023

View reviewed changes

josecelano mentioned this pull request Mar 9, 2023

Rebase of PR 170 #226

Merged

josecelano closed this Mar 9, 2023

mickvandijke deleted the 156-fix-random-test-failure-due-to-port-collision branch March 14, 2023 07:36

Refactor: move shared test functionality to a dev-dependency #170

Refactor: move shared test functionality to a dev-dependency #170

Conversation

mickvandijke commented Feb 2, 2023 • edited Loading

Prerequisites

mickvandijke commented Feb 16, 2023

josecelano commented Feb 16, 2023 • edited Loading

mickvandijke commented Feb 20, 2023

mickvandijke commented Feb 21, 2023

josecelano commented Feb 21, 2023

mickvandijke commented Feb 22, 2023

da2ce7 commented Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecelano Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecelano left a comment

Choose a reason for hiding this comment

josecelano commented Mar 2, 2023 • edited Loading

mickvandijke commented Mar 2, 2023 • edited Loading

mickvandijke commented Mar 2, 2023

josecelano commented Mar 2, 2023 • edited Loading

mickvandijke commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecelano Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

mickvandijke Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickvandijke Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecelano commented Mar 8, 2023

mickvandijke commented Mar 9, 2023

josecelano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickvandijke commented Mar 9, 2023

josecelano commented Mar 9, 2023

mickvandijke commented Mar 9, 2023

josecelano left a comment

Choose a reason for hiding this comment

mickvandijke commented Mar 9, 2023

josecelano commented Mar 9, 2023

josecelano commented Mar 9, 2023

mickvandijke commented Feb 2, 2023 •

edited

Loading

josecelano commented Feb 16, 2023 •

edited

Loading

da2ce7 commented Feb 22, 2023 •

edited

Loading

josecelano Mar 1, 2023 •

edited

Loading

josecelano commented Mar 2, 2023 •

edited

Loading

mickvandijke commented Mar 2, 2023 •

edited

Loading

josecelano commented Mar 2, 2023 •

edited

Loading

josecelano Mar 3, 2023 •

edited

Loading

mickvandijke Mar 3, 2023 •

edited

Loading

mickvandijke Mar 3, 2023 •

edited

Loading