-
Notifications
You must be signed in to change notification settings - Fork 4k
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(core): move test related files from src to test directory #17023
base: main
Are you sure you want to change the base?
Conversation
@@ -5,7 +5,7 @@ | |||
import 'package:firebase_auth_platform_interface/firebase_auth_platform_interface.dart'; | |||
import 'package:firebase_auth_platform_interface/src/method_channel/method_channel_firebase_auth.dart'; | |||
import 'package:firebase_core_platform_interface/firebase_core_platform_interface.dart'; | |||
import 'package:firebase_core_platform_interface/test.dart'; | |||
import '../../../firebase_core/firebase_core_platform_interface/test/test.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is why we previously had flutter_test in dependencies as opposed to dev dependencies so we can share it with other packages. But, I don't think this matters because it will be excluded when publishing to pub.dev. Therefore, relative paths should be fine for other packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double check, I'll see if @Lyokone has any feedback on the matter 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum I'm wondering how it will fix the issue, we have not changed anything in the pubspec.yaml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lyokone, I have done that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice missed this file :)
I don't think this will work, it seems tests/ directory is published as well and has to be explicitly ignored in |
Related Issues
Fixes #17001
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).melos run analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?