feat: new optional loadFonts parameter

[EArminjon]

Description

fix memory issue along multiple call to loadFonts.
#76

As a developer, I just need to put loadFonts: false and call the following code before my multiple goldenTest().

setUpAll(() async {
    await loadFonts();
  });

This fix also increase test speed, it's insane.

Type of Change

New optional parameters

• ✨ New feature (non-breaking change which adds functionality)
• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ❌ Breaking change (fix or feature that would cause existing functionality to change)
• 🧹 Code refactor
• ✅ Build configuration change
• 📝 Documentation
• 🗑️ Chore

[EArminjon]

Maybe an other solution can be to check inside loadFonts if the fonts already exist. I didn't know if it's possible.

[TesteurManiak]

I find it a bit cumbersome to have to specify goldenTest(loadFonts: false) every time when you might want to disable it globally.
Maybe you could consider injecting the loadFonts value with an environment variable 🤔

For example:

flutter test --dart-define=LOAD_GOLDEN_FONTS=false

The implementation in golden_test.dart could be:

const _loadFontsDefault = bool.fromEnvironment('LOAD_GOLDEN_FONTS', defaultValue: true);

// ...

Future<void> goldenTest({
  // ...
  bool? loadFonts,
}) async {
  // ...

  goldenTestAdapter.setUp(
    () => _setUpGoldenTests(mustLoadFonts: loadFonts ?? _loadFontsDefault),
  );
}

[EArminjon]

I find it a bit cumbersome to have to specify goldenTest(loadFonts: false) every time when you might want to disable it globally. Maybe you could consider injecting the loadFonts value with an environment variable 🤔

For example:

flutter test --dart-define=LOAD_GOLDEN_FONTS=false

The implementation in golden_test.dart could be:

const _loadFontsDefault = bool.fromEnvironment('LOAD_GOLDEN_FONTS', defaultValue: true);

// ...

Future<void> goldenTest({
  // ...
  bool? loadFonts,
}) async {
  // ...

  goldenTestAdapter.setUp(
    () => _setUpGoldenTests(mustLoadFonts: loadFonts ?? _loadFontsDefault),
  );
}

I think we can find a better solution. I create this PR as a quick fix because i (and my team) couldn't use this package.
Defining env variable is maybe not perfect. If someone forgot it or just run flutter test, he can encounter issues.

[TesteurManiak]

Defining env variable is maybe not perfect. If someone forgot it or just run flutter test, he can encounter issues.

True, maybe adding a property to AlchemistConfig or PlatformGoldensConfig should be the way to go 🤔

[bmt-github-policybot]

This pull request has been automatically closed because it has not been updated in the last 3 months. 😪

If you still need this change, please reopen it and update it within the next 3 months.

Thanks for helping keep our house in order!

[EArminjon]

Don't close please

[EArminjon]

Should I add the parameter in this config ?

[samandmoore]

Sorry about that! We have to make some time to dig into this

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (cd09414) to head (8040f8a).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #101   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          562       563    +1     
=========================================
+ Hits           562       563    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bmt-github-policybot]

This pull request has been automatically closed because it has not been updated in the last month. 😪

If you still need this change, you can reopen it.

Thanks for helping keep our house in order!

[pedromassango]

commenting for visibility

[jeroen-meijer]

What I'm gathering from this is, loadFonts only needs to be called once per group/test/instance (which one is it?). So then shouldn't we move loadFonts into some setup logic that is guaranteed to only be run once per group/test/instance?

This doesn't seem like something users should handle, but instead something that Alchemist needs to keep into account by default. I don't see a use-case for a feature where users can toggle loadFonts on or off per test or group -- it should either happen once or not at all (if a user desires the latter).

@samandmoore @btrautmann @Kirpal @EArminjon Can any of you confirm this thinking?

[pedromassango]

What I'm gathering from this is, loadFonts only needs to be called once per group/test/instance (which one is it?). So then shouldn't we move loadFonts into some setup logic that is guaranteed to only be run once per group/test/instance?

This doesn't seem like something users should handle, but instead something that Alchemist needs to keep into account by default. I don't see a use-case for a feature where users can toggle loadFonts on or off per test or group -- it should either happen once or not at all (if a user desires the latter).

@samandmoore @btrautmann @Kirpal @EArminjon Can any of you confirm this thinking?

That is exactly it. Thank you!
If we could make sure it runs one per every flutter test that would be ideal as devs don't tend to change fonts per test/group.

[btrautmann]

What I'm gathering from this is, loadFonts only needs to be called once per group/test/instance (which one is it?). So then shouldn't we move loadFonts into some setup logic that is guaranteed to only be run once per group/test/instance?

This doesn't seem like something users should handle, but instead something that Alchemist needs to keep into account by default. I don't see a use-case for a feature where users can toggle loadFonts on or off per test or group -- it should either happen once or not at all (if a user desires the latter).

@samandmoore @btrautmann @Kirpal @EArminjon Can any of you confirm this thinking?

I agree with the thinking here and agree with @pedromassango that this makes sense once per "suite run". This actually feels similar in terms of granularity to what I mention here, where we're wanting to detect the architecture once and then resolve the golden paths based on that. I mentioned some sort of "global caching mechanism" that may make sense for this use case as well, so maybe in whatever issue we tackle first we could be sure to support additional use cases (if it makes sense to do so).