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

Fix CI errors #37

Merged
merged 2 commits into from
Apr 22, 2019
Merged

Fix CI errors #37

merged 2 commits into from
Apr 22, 2019

Conversation

efortuna
Copy link
Collaborator

@efortuna efortuna commented Apr 22, 2019

No description provided.

@efortuna efortuna requested a review from luigi-rosso April 22, 2019 23:16
@efortuna
Copy link
Collaborator Author

ahhh nooo what is wrong now?

@luigi-rosso
Copy link
Contributor

I think I must not be disposing a timer or an animation controller somewhere.
Screen Shot 2019-04-22 at 4 23 25 PM

@luigi-rosso
Copy link
Contributor

Looking into it!

@efortuna
Copy link
Collaborator Author

okay. I'm going to push this at least

@efortuna efortuna merged commit 6e46f8d into master Apr 22, 2019
@efortuna efortuna deleted the ci branch April 22, 2019 23:28
@luigi-rosso
Copy link
Contributor

luigi-rosso commented Apr 22, 2019

I couldn't find anything yet, but I do see the test failing locally too. All timers seem to be canceled and animations disposed. I couldn't find any delayed futures either.

@filiph
Copy link
Collaborator

filiph commented Apr 22, 2019

This does not fail locally for me. I noticed this is running on master of Flutter. Might that be the problem? See also #32 (comment).

@luigi-rosso
Copy link
Contributor

Oh interesting, yes we are using master. What should we change it to? You can change this directly here:
https://github.com/2d-inc/dev_rpg/blob/101bd8232fdd91c71e46a3ecc157cfed1d2d5d1d/.circleci/config.yml#L21

@brianegan
Copy link
Collaborator

Per #41, I'd say let's go with the dev branch. It has the required Dart version we need but might be just a little more stable than master.

@brianegan
Copy link
Collaborator

brianegan commented Apr 23, 2019

Update: Even on dev branch this test fails (also fails locally for me). See #45

@filiph
Copy link
Collaborator

filiph commented Apr 23, 2019

Ok, I have a new theory. The smoke test fails because it's waiting for Flare assets (there's a long standing issue in which assets are not loading by default with the test runner, and I see that Flare is waiting for those assets without cancelling when disposed).

Over at #39 I'm adding some more tests (mostly for show, tbh — I need tests for our I/O session) and I'm skipping the smoke test. If I have time this week, I can take a closer look at the smoke test and un-skip it. But I don't want us to wait on that.

I also switched to dev branch instead of master. On master, I saw a really bizarre Uint16List/Int32List error that I don't think we want to deal with. master is not nearly stable enough for serious development, it's literally tip of tree.

Now it's green over at #39.

@filiph filiph mentioned this pull request Apr 23, 2019
@luigi-rosso
Copy link
Contributor

The Uint16/Int32List error is a breaking change that was committed to the engine yesterday:
flutter/engine@ed1f3fd

We're also tracking it here:
2d-inc/Flare-Flutter#75 (comment)

In the meantime I can take a look at the asset loading disposing properly.

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.

4 participants