Skip to content

refactor: update crate dependencies#3216

Merged
didier-wenzek merged 1 commit intothin-edge:mainfrom
didier-wenzek:fix/cargo-update
Oct 30, 2024
Merged

refactor: update crate dependencies#3216
didier-wenzek merged 1 commit intothin-edge:mainfrom
didier-wenzek:fix/cargo-update

Conversation

@didier-wenzek
Copy link
Copy Markdown
Contributor

Proposed changes

  • cargo update
  • fix mockito-based tests broken after dependencies update
  • fix actor runtime test broken after dependencies update

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 30, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
528 0 2 528 100 1h32m55.737206999s

Copy link
Copy Markdown
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

create() was not replaced by create_async() in two files.

let _ = env_logger::try_init();
let mut server = mockito::Server::new();
let mut server = mockito::Server::new_async().await;
let _mock = server
Copy link
Copy Markdown
Member

@rina23q rina23q Oct 30, 2024

Choose a reason for hiding this comment

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

Don't we need to change from create() to create_async() in this file?

       let _mock = server
            .mock("GET", "/hello")
            .match_header("Authorization", "Bearer test-token")
            .with_status(204)
            .create(); // shouldn't it be `.create_async().await;`   ?

Copy link
Copy Markdown
Member

@Bravo555 Bravo555 Oct 30, 2024

Choose a reason for hiding this comment

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

Looking at the crate documentation, there doesn't seem to be much difference between the two, i.e. both will work here, but using .create_async we don't block and await, which would maybe have performance implications when registering many mocks, but in our use case there's no difference as long as .create also works so the difference is if we started any other tasks before, calling .create() would block the only worker thread and block these tasks as well, but only for a short time that it takes to register a mock, so there's not much difference in our case.

Nevermind, didn't read the whole thing, it seems blocking the runtime for any amount of time is a hard error somehow.
From mockito's docs:

Mockito comes with both a sync and an async interface.

In order to write async tests, you’ll need to use the _async methods:

  • Server::new_async
  • Server::new_with_opts_async
  • Mock::create_async
  • Mock::assert_async
  • Mock::matched_async
  • Mock::remove_async

…otherwise your tests will not compile and you’ll see the following error:

Cannot block the current thread from within a runtime.
This happens because a function attempted to block the current thread while the thread is being used to drive asynchronous tasks.

In this case you're right, we need to use .create_async() everywhere

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Bravo555 for the insight. However, I decided to apply @rina23q suggestions to be consistent.

assert_eq!(
error.map(|s| s.replace(char::is_numeric, "")), // ignore the task id
Some("task panicked".to_string())
Some("task panicked with message \"Oh dear\"".to_string())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo: there's two spaces in task panicked

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The whole commit has been dropped

let mut server = mockito::Server::new_async().await;
let _mock = server
.mock("GET", "/not-a-known-url")
.with_status(404)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You missed to apply create_async to the line below. (line 899)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

:-) I will fixed that.

That said this demonstrates that @Bravo555 comment is correct. The tests are working as expected with create() as well ass with create_async().

Copy link
Copy Markdown
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

Approved

With `mockito 1.5`, one has to use the `new_async` and `create_async` methods.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek didier-wenzek added this pull request to the merge queue Oct 30, 2024
Merged via the queue into thin-edge:main with commit 75d772b Oct 30, 2024
@didier-wenzek didier-wenzek deleted the fix/cargo-update branch October 30, 2024 16:43
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.

3 participants