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

Bump mockito and regenerate mocks #22

Merged
merged 5 commits into from
Dec 14, 2023
Merged

Conversation

srawlins
Copy link
Contributor

@srawlins srawlins commented Dec 8, 2023

The existing mock files contain a dangerous Future.value call, where, for a potentially non-nullable type S, we call Future<S>.value(null). But S may be non-nullable, so this is dangerous. The dart analyzer will start reporting this. See dart-lang/sdk#53253. See this issue in Flutter for a reference to the fixes made in Flutter.

This PR just bumps mockito, and regenerates the mocks, where it starts generating safer code. For an example, see in the diff on line 487 in the old copy. (I wish the diff were more isolated, but import prefixes changed and it looks like some other things.)

@srawlins
Copy link
Contributor Author

Ping, this is blocking a fix for dart-lang/sdk#53253.

@Goddchen
Copy link
Owner

Looks like the newly generated code produces some issues with the analyzer and the tests. Let me have a look.

@Goddchen
Copy link
Owner

Seems like somehow it won't compile with mockito 5.4.0, but needs at least 5.4.2, I guess. Not sure why the CI job says mockito 5.4.0 (5.4.3 available) and is not using mockito 5.4.3 (pubspec.lock is not part of the repo, so it should download the latest versions). Any ideas?

@Goddchen
Copy link
Owner

If you simply upgrade the dependency from mockito: ^5.4.0 to mockito: ^5.4.3, that would fix the issues.

@srawlins
Copy link
Contributor Author

Sorry about that! Thanks for following up and digging into that. I've bumped it.

@Goddchen
Copy link
Owner

Goddchen commented Dec 14, 2023

Because dart_rfb depends on mockito >=5.4.1 which requires SDK version >=2.19.0 <4.0.0, version solving failed.

Welcome to dependency hell... I guess it's fine if we update the required dart SDK version to >=2.19.0. Upgrade it in pubspec.yaml and in the GitHub Actions files.

@srawlins
Copy link
Contributor Author

OK, done! Thanks!

@Goddchen
Copy link
Owner

Because dart_rfb depends on mockito >=5.4.3 which requires SDK version ^3.1.0, version solving failed..

Like I said, dependency hell... Can you upgrade it to ^3.1.0?

@Goddchen
Copy link
Owner

Goddchen commented Dec 14, 2023

And maybe just run dart analyze this time locally before pushing your commit, to save us some time 😉

@srawlins
Copy link
Contributor Author

Sorry about the back-and-forth. I don't have a copy of Dart 2.19.0 or earlier so wouldn't have caught that issue. But it analyzes cleanly with Dart 3.2.3 🤷

Copy link
Owner

@Goddchen Goddchen left a comment

Choose a reason for hiding this comment

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

LGTM, ty!

@Goddchen Goddchen merged commit 69d5639 into Goddchen:main Dec 14, 2023
3 checks passed
@srawlins
Copy link
Contributor Author

Thanks a million for your patience 🙏

@srawlins srawlins deleted the mockito-nullable branch December 14, 2023 20:52
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.

2 participants