Add more tests to FDA check

[MarkusPettersson98]

This PR touches up the FDA check code just a little bit more to ensure that no obvious bugs are still present.

---

This change is 

[dlon]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

talpid-core/src/split_tunnel/macos/process.rs line 616 at r1 (raw file):

    #[tokio::test]
    async fn test_parse_logger_status_timeout() {
        let need_fda = parse_logger_status(

This test doesn't seem to test any time out now (since it immediately prints to stdout/stderr). It is identical to test_parse_logger_status_immediate_exit.

Would something like make more sense?

        let stdout = output("This will never be printed\n", Duration::MAX);
        let stderr = output(
            "ES_NEW_CLIENT_RESULT_ERR_NOT_PERMITTED\n",
            EARLY_FAIL_TIMEOUT + Duration::from_secs(1),
        );

Though I'm not sure if it's particularly valuable given that it's just a special case of test_parse_logger_status_hogged. Removing it might be fine?

---

talpid-core/src/split_tunnel/macos/process.rs line 657 at r1 (raw file):

            EARLY_FAIL_TIMEOUT / 2,
        );
        tokio::time::resume();

Are start and resume necessary here? I think it should just work if you only set the start_paused attribute: #[tokio::test(start_paused = true)]

[MarkusPettersson98]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dlon)

---

talpid-core/src/split_tunnel/macos/process.rs line 616 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

This test doesn't seem to test any time out now (since it immediately prints to stdout/stderr). It is identical to test_parse_logger_status_immediate_exit.

Would something like make more sense?

        let stdout = output("This will never be printed\n", Duration::MAX);
        let stderr = output(
            "ES_NEW_CLIENT_RESULT_ERR_NOT_PERMITTED\n",
            EARLY_FAIL_TIMEOUT + Duration::from_secs(1),
        );

Though I'm not sure if it's particularly valuable given that it's just a special case of test_parse_logger_status_hogged. Removing it might be fine?

It is a special case of test_parse_logger_status_hogged, so I think we can simply remove this one. Renaming it to test_parse_logger_status_timeout though 😊

---

talpid-core/src/split_tunnel/macos/process.rs line 657 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Are start and resume necessary here? I think it should just work if you only set the start_paused attribute: #[tokio::test(start_paused = true)]

We can get rid of start in favor of #[tokio::test(start_paused = true)], but resume seems relevant to me because time should not be frozen during the subsequent call to parse_logger_status. Maybe I understand the tokio::time test utils wrong tho

[dlon]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewable status: complete! all files reviewed, all discussions resolved

---

talpid-core/src/split_tunnel/macos/process.rs line 657 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

We can get rid of start in favor of #[tokio::test(start_paused = true)], but resume seems relevant to me because time should not be frozen during the subsequent call to parse_logger_status. Maybe I understand the tokio::time test utils wrong tho

It automatically advances timers when there's no other work to do. Maybe that's less correct here?