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.

Sign up for GitHub

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

Jump to bottom

feat: allow external packages to await for game to be loaded #1699

Merged
merged 6 commits into from
Jun 6, 2022
Merged

feat: allow external packages to await for game to be loaded #1699

merged 6 commits into from
Jun 6, 2022

Conversation

renancaraujo
Copy link
Member

Description

When using testWidgets to test pages/widgets that renders a game, one may test a widget in a state after the game has loaded.

This introduces toBeLoaded method to be called by widget tests that evaluate things that happens after the game "onLoad" cycle.

await tester.pumpWidget(GameWidget(game: game));
await game.toBeLoaded(); // ⚠️ calling this before pumping the game into a gameWidget throws assertion error. 
await tester.pump();

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • [] I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

@renancaraujo renancaraujo changed the title feat: allow external apckages to await for game to be loaded feat: allow external packages to await for game to be loaded Jun 5, 2022
renancaraujo
renancaraujo commented Jun 5, 2022
View reviewed changes
packages/flame/test/game/flame_game_test.dart
expect(hasAttached, isTrue);
});
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Example of a test that skips to the render box being attached.

@renancaraujo renancaraujo marked this pull request as ready for review June 5, 2022 14:11
@renancaraujo renancaraujo force-pushed the renan/allow-awaiting-load-future branch from a6274e1 to f007cd5 Compare June 5, 2022 14:39
spydon
spydon reviewed Jun 6, 2022
View reviewed changes
packages/flame/lib/src/game/mixins/game.dart Outdated
return true;
}(),
);
return _onLoadFuture = onLoad();
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be stored outside of the method right? Because we should only load before the first time a component is mounted.

Copy link
Member

Choose a reason for hiding this comment

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

And if it is stored outside of the method, this method doesn't really do anything anymore right? Except setting _debugOnLoadStarted to true, but it doesn't actually know that the called of this method awaits the future.

Copy link
Member

@spydon spydon Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I guess since onLoadFuture could be internal we could guarantee that ourselves though.

st-pasha
st-pasha reviewed Jun 6, 2022
View reviewed changes
packages/flame/lib/src/game/mixins/game.dart
/// To be used for tests that needs to evaluate the game after it has been
/// loaded by the game widget.
@visibleForTesting
Future<void>? toBeLoaded() {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's conceptually the same future as Component.loaded, isn't it? Why not name it the same?

Copy link
Member

Choose a reason for hiding this comment

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

I thought that would be the most sensible first too, but since this is a mixin it will override the components implementation when we mix Game with games that extend Component, like FlameGame.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense.
I guess we can eventually consider separating FlameGame from the Component so that these kinds of issues don't appear again (AFAIR we definitely ran into similar problems before).

@spydon spydon merged commit a15eda0 into main Jun 6, 2022
@spydon spydon deleted the renan/allow-awaiting-load-future branch June 6, 2022 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spydon spydon spydon approved these changes

@st-pasha st-pasha st-pasha left review comments

@erickzanardo erickzanardo erickzanardo approved these changes

@luanpotter luanpotter Awaiting requested review from luanpotter

@wolfenrain wolfenrain Awaiting requested review from wolfenrain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@renancaraujo @spydon @erickzanardo @st-pasha