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

tests: library tests to play multiple sources #1333

Merged
merged 50 commits into from Dec 25, 2022
Merged

Conversation

Gustl22
Copy link
Collaborator

@Gustl22 Gustl22 commented Nov 23, 2022

Description

  • feat: Lib Tests to play multiple sources (simultaneously / consecutively)

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs:, chore: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation and added dartdoc comments with ///, where necessary.
  • I have updated/added relevant examples in example.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

TODO

@Gustl22 Gustl22 marked this pull request as ready for review December 14, 2022 11:18
@Gustl22 Gustl22 marked this pull request as draft December 18, 2022 21:52
@Gustl22 Gustl22 marked this pull request as ready for review December 20, 2022 19:51
@Gustl22 Gustl22 marked this pull request as draft December 20, 2022 22:58
Copy link
Member

@luanpotter luanpotter 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 continuously improving our tests!
I like the idea of the local server.
overall LGTM

just one concern about the error stream change, I wonder if we can split that off

@Gustl22 Gustl22 force-pushed the feat/test-multiple-sources branch 4 times, most recently from d25a1eb to d37a8c0 Compare December 22, 2022 09:10
@Gustl22 Gustl22 changed the title tests(lib): play multiple sources tests: library tests, local test server, update build workflow Dec 23, 2022
@Gustl22 Gustl22 marked this pull request as ready for review December 23, 2022 11:58
@Gustl22
Copy link
Collaborator Author

Gustl22 commented Dec 23, 2022

I can also split the server and workflow stuff from this PR if wanted :) Idk if running the tests twice is needed. We could only run the tests on the PRs and the build when pushing to main

@spydon
Copy link
Member

spydon commented Dec 23, 2022

I can also split the server and workflow stuff from this PR if wanted :) Idk if running the tests twice is needed. We could only run the tests on the PRs and the build when pushing to main

I think both are fine, since when it is running on main after a merge we're not really waiting for it anyways, but I also think we don't check the results of that. :)
What is good with having it running on main top is that you can set it to run periodically when the project is less active and then you'll see if it breaks with any new Flutter or package version.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@Gustl22
Copy link
Collaborator Author

Gustl22 commented Dec 23, 2022

I think both are fine, since when it is running on main after a merge we're not really waiting for it anyways, but I also think we don't check the results of that. :)

Yeah, doesn't gain any profit from time perspective. More was a concern of free gh action minutes, but as long as we don't get in trouble, we could run these.

What about the build tests for the PR? Do you think they are necessary? The tests shouldn't be able to run without building anyways I guess.

What is good with having it running on main top is that you can set it to run periodically when the project is less active and then you'll see if it breaks with any new Flutter or package version.

I more like the approach of a dependency bot, and "hardcode" the version values, to get rid of unexpected failures. This works for Flutter version or even the GH-Action dependencies. So the tests do only run when an update is available and the developer can decide if want to merge the upgrade.

@Gustl22 Gustl22 marked this pull request as draft December 23, 2022 21:57
@Gustl22 Gustl22 mentioned this pull request Dec 23, 2022
7 tasks
@spydon
Copy link
Member

spydon commented Dec 23, 2022

Yeah, doesn't gain any profit from time perspective. More was a concern of free gh action minutes, but as long as we don't get in trouble, we could run these.

I don't think we have that restriction since we're an open source organisation.

What about the build tests for the PR? Do you think they are necessary? The tests shouldn't be able to run without building anyways I guess.

Yeah, I guess they aren't necessary.

I more like the approach of a dependency bot, and "hardcode" the version values, to get rid of unexpected failures. This works for Flutter version or even the GH-Action dependencies. So the tests do only run when an update is available and the developer can decide if want to merge the upgrade.

I think a dependency bot works well for normal dependencies, but not that well for Flutter and Dart versions since it is recommended to support a range that includes all minor bumps too.

@Gustl22
Copy link
Collaborator Author

Gustl22 commented Dec 24, 2022

I don't think we have that restriction since we're an open source organisation.

I think for organizations it's 2000 minutes.
https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions#included-storage-and-minutes.

Ahh sry, you're right. It's free for public repos :)

I think a dependency bot works well for normal dependencies, but not that well for Flutter and Dart versions since it is recommended to support a range that includes all minor bumps too.

I'm not quite sure, if I get the proposal right. I think for both variants the developer needs to take action ASAP, also for minor bumps. It's sometimes unfortunate, if you have a PR open and suddenly the code doesn't work anymore on the CI because the versions (Flutter or OS versions) have changed, like happened recently in ubuntu. And one needs to fix the os first in another or even worse, in the current PR, and mix the dependency fixes with the feature.

With a bot, for every bump the build should run and the developer immediately sees the failures in the PR of the bot. And the dev can make the fixes in there, where it belongs.

That are only my thoughts, I'm also fine with other approaches. We can also discuss this later or in the according PR, as we currently do not implement any of these strategies.

Merry Christmas 🎁

@Gustl22
Copy link
Collaborator Author

Gustl22 commented Dec 24, 2022

I also splitted the test server from this PR, see #1354

@spydon
Copy link
Member

spydon commented Dec 24, 2022

I'm not quite sure, if I get the proposal right. I think for both variants the developer needs to That are only my thoughts, I'm also fine with other approaches. We can also discuss this later or in the according PR, as we currently do not implement any of these strategies.

Merry Christmas 🎁

Yeah, I guess there are pros and cons to both solutions, I'm fine with any.

Merry christmas! 🎅

# Conflicts:
#	.github/workflows/test.yml
#	packages/audioplayers/example/lib/tabs/sources.dart
@Gustl22 Gustl22 changed the title tests: library tests, local test server, update build workflow tests: library tests to play multiple sources Dec 24, 2022
…sources

# Conflicts:
#	.github/workflows/build.yml
#	.github/workflows/pull-request.yml
#	.github/workflows/test.yml
@Gustl22 Gustl22 marked this pull request as ready for review December 24, 2022 22:33
@Gustl22 Gustl22 requested a review from spydon December 24, 2022 22:34
@Gustl22 Gustl22 merged commit 81958a6 into main Dec 25, 2022
@Gustl22 Gustl22 deleted the feat/test-multiple-sources branch December 25, 2022 23:30
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.

None yet

3 participants