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

firebase-test-labs 0.0.2 #4077

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AnthonyDadeWT
Copy link

@AnthonyDadeWT AnthonyDadeWT commented Jan 9, 2024

TagCheck

What to do if the build fails?

At the moment contributors do not have access to the CI workflow triggered by StepLib PRs. In case of a failed build, we ask for your patience. Maintainers of Bitrise Steplib will sort it out for you or inform you if any further action is needed.

New Pull Request Checklist

Please mark the points which you did / accept.

  • [ x ] I will not move an already shared step version's tag to another commit
  • [ x ] I read the Step Development Guideline
  • [ x ] I have a test for my Step, which can be run with bitrise run test (in the step's repository)
  • [ x ] I did run bitrise run audit-this-step (in the step's repository - note, if you don't have this workflow in your bitrise.yml, you can copy it from the step template.)
  • [ x ] I read and accept the Abandoned Step policy

New Step
Thank you for the new Step share! The CI check might will fail due to our extended validation engine. Nothing to worry about yet, we will get back to you shortly.

@CLAassistant
Copy link

CLAassistant commented Jan 9, 2024

CLA assistant check
All committers have signed the CLA.

@bitrise-coresteps-bot
Copy link
Collaborator

Don't forget to follow semantic versioning! Breaking changes (including step inputs) should be in major versions only.

step.yml changes compared to previous version:

@AnthonyDadeWT
Copy link
Author

Let me know of any questions, suggestions, or action items I need to complete. Thank you!

Copy link
Contributor

@ofalvai ofalvai left a comment

Choose a reason for hiding this comment

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

Hello @AnthonyDadeWT 👋

A general observation before doing a deeper review of this step: it's probably not a good idea to do both the Flutter build and the Firebase testing in a single step. There are existing Flutter steps for building iOS or Android artifacts, this step should probably work with the outputs of the build step.

It's easier to debug build issues when one step is doing one thing only. Also, this step could be useful for non-Flutter projects too, if it was working with standard app artifacts.

@AnthonyDadeWT
Copy link
Author

AnthonyDadeWT commented Feb 21, 2024

@ofalvai

Hello, and thank you for your review!

A general observation before doing a deeper review of this step: it's probably not a good idea to do both the Flutter build and the Firebase testing in a single step. There are existing Flutter steps for building iOS or Android artifacts, this step should probably work with the outputs of the build step

This is a good point. I did implement this for Android, on line 101 of my step.sh starts the logic of checking whether or not we need to build an APK based on if $BITRISE_APK_PATH exists. However I don't think this would actually be possible on the iOS side, because according to flutter's documentation on deploying integration tests to FTL , you must first run flutter build ios $integration_test_path --release for this process to actually work. So what do you think about this case? Maybe a "Flutter Build for Testing" Step should be created in conjunction with this step to use together like you said.

Now that I think about it, I guess users would be able to pass this in as an additional argument to the Flutter Build step. Although it is also required to run ./gradlew app:assembleDebug -Ptarget=$integration_test_path for the Android part of this to work.

It's easier to debug build issues when one step is doing one thing only. Also, this step could be useful for non-Flutter projects too, if it was working with standard app artifacts.

This is a thought I had as well since this Step should be compatible. However, I was afraid that since Bitrise has "Device Testing" Steps for iOS and Android already, I feared that it would be considered as a duplicate step. Although I will say there are major differences from the Device Testing Steps and the one I've created here, where this Step uses the user's Firebase project, runs the tests asynchronously, and the results are displayed there on Firebase instead of Bitrise.

@ofalvai
Copy link
Contributor

ofalvai commented Feb 23, 2024

I see, thank you for the extra context and details :) In this case, I'm thinking maybe the best compromise would be to add "Flutter" to the step title to avoid confusion and differentiate it from other Firebase steps. What do you think about this compromise?

@AnthonyDadeWT
Copy link
Author

Yes that makes sense to me. I thought I couldn't have a platform's name in the title due to the rules on Steps but I see other Steps have Platforms in the name.

Here is my plan for updating this step to follow your guidance.

Change to accept outputs from

Flutter Build (for a release build IPA)
Android Build for UI Testing (for an APK and TestAPK)
Xcode Build for testing (.xctestrun)

Then modify the Step to accept these outputs and only run the execution of sending these items to FTL.

Update documentation to guide user to use these Steps before this Step in a workflow.

Change name to "Flutter Device Testing" ? - open to suggestions here

Let me know how that all sounds. It will take me some time to get this done.

@ofalvai
Copy link
Contributor

ofalvai commented Mar 8, 2024

@AnthonyDadeWT the plan looks good to me, thanks for outlining this!

Naming: I would keep "Firebase" in the title because of your previous point that it does something similar to the first-party step, but with the user's Firebase account. So anyone who wants to set up Firebase testing in a custom way would find your step.

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

4 participants