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

test(cat-voices): adds web tests for flutter unit tests #1948

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

kukkok3
Copy link
Contributor

@kukkok3 kukkok3 commented Mar 3, 2025

Description

This PR is introducing a new melos script to run flutter unit tests on web

Related Issue(s)

List the issue numbers related to this pull request.

e.g., Closes #1, Resolves #1 Fixes #1

Description of Changes

Provide a clear and concise description of what the pull request changes.

Breaking Changes

Describe any breaking changes and the impact.

Screenshots

If applicable, add screenshots to help explain your changes.

Related Pull Requests

If applicable, list any related pull requests.

e.g., #1, #1

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@kukkok3 kukkok3 marked this pull request as ready for review March 3, 2025 12:37
@kukkok3 kukkok3 marked this pull request as draft March 3, 2025 12:37
@kukkok3 kukkok3 marked this pull request as ready for review March 3, 2025 12:37
@kukkok3 kukkok3 added do not merge yet PR is not ready to be merged yet do not review yet Do not review yet labels Mar 3, 2025
@kukkok3 kukkok3 self-assigned this Mar 3, 2025
@dtscalac
Copy link
Contributor

dtscalac commented Mar 3, 2025

@kukkok3 The --platform chrome option is soon going to be removed completely without any replacement, we cannot use it, otherwise on next upgrade these tests will break

Copy link
Contributor

github-actions bot commented Mar 5, 2025

Test Report | ${\color{lightgreen}Pass: 638/643}$ | ${\color{red}Fail: 0/643}$ |

@kukkok3
Copy link
Contributor Author

kukkok3 commented Mar 6, 2025

@kukkok3 The --platform chrome option is soon going to be removed completely without any replacement, we cannot use it, otherwise on next upgrade these tests will break

@dtscalac Sorry I just saw your comment.

  • I dont want to remove the native tests, I'll just add a target to run tests on web as well. If the platform chrome will be removed we can disable that target.
  • I looked trough the flutter repo and the only issue I could find related to removing the flag is this one From what I can read I dont think the flag will be removed any time soon adn they are well aware that flag is used outside flutter as well.
  • There are recent issues opened regarding bugs with this flag and seems like flutter devs are still supporting it
  • We worked under the assumption that flutter works the same on every platform. As it turns out this is not true. For example ( besides the issue with numbers ) we use a database that uses a different library to support native or web. Turns out our tests are using a native implementation while in the app we are using the web implementation. How I'm supposed to test this stuff? How do I know what changes between flutter compiled for native and flutter compiled for web? I could not find such a list.
    I'm open to suggestions but so far I cannot find any other option other then run unit tests on web as well

@kukkok3 kukkok3 linked an issue Mar 6, 2025 that may be closed by this pull request
@kukkok3 kukkok3 changed the title test: run test with platform chrome test(cat-voices): adds web tests for flutter unit tests Mar 14, 2025
@kukkok3 kukkok3 added review me PR is ready for review and removed do not merge yet PR is not ready to be merged yet do not review yet Do not review yet labels Mar 18, 2025
@@ -98,9 +97,13 @@ check-license:
RUN melos run license-check

# Run unit tests
test-unit:
test-unit-native:
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +21 to +22
flutter_test:
sdk: flutter
Copy link
Contributor

Choose a reason for hiding this comment

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

This package was designed as pure dart package, it means it should have no dependency on flutter. For testing we use test package, not flutter_test. What's the reason to include the flutter_test package?

Comment on lines +29 to +30
flutter_test:
sdk: flutter
Copy link
Contributor

Choose a reason for hiding this comment

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

This package was designed as pure dart package, it means it should have no dependency on flutter. For testing we use test package, not flutter_test. What's the reason to include the flutter_test package?

Comment on lines +7 to +8
flutter_test:
sdk: flutter
Copy link
Contributor

Choose a reason for hiding this comment

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

this package has no tests and won't have them, do we need the test framework here?

Comment on lines +31 to +32
flutter_test:
sdk: flutter
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be required for this package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me PR is ready for review
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

🛠️ [TASK] : Run flutter tests on web First
2 participants