Skip to content

Conversation

@OlegNovosad
Copy link
Contributor

@OlegNovosad OlegNovosad commented Jul 11, 2022

  • Fixed setting up mocks with the new firebase_core_platform_interface version
  • Updated dependencies
  • Fixed some lint issues

@sitatec sitatec self-requested a review July 12, 2022 15:56
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

Codecov Report

Merging #6 (e5982df) into main (5b54199) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #6   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files           4        4           
  Lines          85       85           
=======================================
  Hits           80       80           
  Misses          5        5           

Help us with your feedback. Take ten seconds to tell us how you rate us.

@Zohenn
Copy link
Contributor

Zohenn commented Jul 20, 2022

Might as well move flutter_lints and fluter_test to dev_dependencies in pubspec.yaml, I suppose they belong there actually.

@OlegNovosad
Copy link
Contributor Author

Might as well move flutter_lints and fluter_test to dev_dependencies in pubspec.yaml, I suppose they belong there actually.

Done.

- Use the latest stable flutter
- Changed `runs-on` from `macos-latest` to `ubuntu-latest` to minimize actions time spent as macos minute is treated as 10 ubuntu minutes
@OlegNovosad
Copy link
Contributor Author

@sitatec I've also updated Github Actions build script with next improvements:

  • Use the latest stable flutter
  • Changed runs-on from macos-latest to ubuntu-latest to minimize actions time spent as macos minute is treated as 10 ubuntu minutes

@Zohenn
Copy link
Contributor

Zohenn commented Jul 24, 2022

Cool, that's what caused my PR to fail to build. I wanted to make similar changes, but now I'll wait for @sitatec to merge your PR.

Copy link
Owner

@sitatec sitatec left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @OlegNovosad. We just need to revert one change before merging.

Comment on lines 19 to 24
test("Should get reference using the deprecated reference() method", () {
expect(
MockFirebaseDatabase.instance.reference().child("deprecated").path,
MockFirebaseDatabase.instance.ref().child("deprecated").path,
MockDatabaseReference().child("deprecated").path,
);
});
Copy link
Owner

Choose a reason for hiding this comment

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

We need to revert this change. This test is for the reference method not for the ref one, the ref method has it own tests.
Although the reference method is deprecated, it hasn't been removed yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense @sitatec ! Updated.

@OlegNovosad OlegNovosad requested a review from sitatec July 24, 2022 12:46
@sitatec sitatec merged commit 53505d1 into sitatec:main Jul 24, 2022
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