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

feat: mock mode #2248

Merged
merged 4 commits into from
Aug 16, 2024
Merged

feat: mock mode #2248

merged 4 commits into from
Aug 16, 2024

Conversation

Tienisto
Copy link
Contributor

@Tienisto Tienisto commented Aug 15, 2024

Changes

Fixes #2247.

Checklist

  • An issue to be fixed by this PR is listed above.
  • New tests are added to ensure new features are working. Please refer to this page to see how to add a test.
  • ./frb_internal precommit --mode slow (or fast) is run (it internal runs code generator, does auto formatting, etc).
  • If this PR adds/changes features, documentations (in the ./website folder) are updated.
  • CI is passing. Please refer to this page to see how to solve a failed CI.

Remark for PR creator

  • ./frb_internal --help shows utilities for development.
  • If fzyzcjy does not reply for a few days, maybe he just did not see it, so please ping him.

Copy link

welcome bot commented Aug 15, 2024

Hi! Thanks for opening this pull request! 😄

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 15, 2024

Good job! Some nits

  1. I am wondering, for user-facing API, whether we should make it a separate function initMock, or the same function with an extra arg init({..., bool mock})? (I think both have pros and cons...)
  2. To fix CI, try ./frb_internal precommit-generate (which takes at least several minutes to run since it runs codegen in debug mode for many folders)

@Tienisto
Copy link
Contributor Author

Tienisto commented Aug 15, 2024

Thanks for the quick response!

Actually, I had it like init({..., bool mock}) originally. However, I thought it would be unclear what parameters are used and I didn't like that api is optional while it is not when mock is true.
A separate function would be more type safe in this case.
I am fine with both solutions though.

Edit: I've run ./frb_internal precommit-generate but it doesn't seem to fix all issues. Do you know what's missing?

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 15, 2024

You are welcome!

However, I thought it would be unclear what parameters are used and I didn't like that api is optional while it is not when mock is true.
A separate function would be more type safe in this case.

Looks pretty reasonable.

I've run ./frb_internal precommit-generate but it doesn't seem to fix all issues. Do you know what's missing?

Looks like need to edit https://github.com/fzyzcjy/flutter_rust_bridge/tree/master/frb_codegen/assets/integration_template/shared (e.g. copy-paste/modify things according to the CI hint) which is the template

@Tienisto
Copy link
Contributor Author

Thanks! I am currently not at home so feel free to add it. Otherwise, I will add it tomorrow

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 16, 2024

@all-contributors please add @Tienisto for code

Copy link
Contributor

@fzyzcjy

I've put up a pull request to add @Tienisto! 🎉

@fzyzcjy fzyzcjy merged commit bed384d into fzyzcjy:master Aug 16, 2024
135 checks passed
Copy link

welcome bot commented Aug 16, 2024

Hi! Congrats on merging your first pull request! 🎉

@Tienisto Tienisto deleted the feature/mock-mode branch August 16, 2024 00:22
@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 16, 2024

@Tienisto Looks like codecov is unhappy (see below; not sure why codecov does not appear in this PR...). Maybe we can add a few lines in https://github.com/fzyzcjy/flutter_rust_bridge/blob/master/frb_example/pure_dart/test/mockability_test.dart to test this code path.

image

https://app.codecov.io/gh/fzyzcjy/flutter_rust_bridge/commit/c5bdedbbb1b222162bdc88d28e96313b687007a8

image

@Tienisto Tienisto mentioned this pull request Aug 16, 2024
5 tasks
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.

Cannot mock RustLib without loading FFI
2 participants