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: organize melos for full local testing, minor fixes so complete runs quickly pass #4331

Merged
merged 5 commits into from
Jan 4, 2021

Conversation

mikehardy
Copy link
Contributor

Description

As a new contributor, it is difficult to say that you have a correct working environment before making your first change, or that after your change you have not broken anything.

The goal of this PR is purely local test oriented then

  • add highly parallel targets to melos.yaml that allow for a fast local runs of the various quality checks
  • add one high level melos.yaml target that can be trusted to fail if anything is wrong
  • fix all problems encountered while exercising those tasks

Related Issues

This is related to the forward-port PR #4249 but these commits may be considered independently and immediately to help developers now, and to shrink that PR to it's core purpose

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@google-cla google-cla bot added the cla: yes label Dec 4, 2020
melos.yaml Show resolved Hide resolved
melos.yaml Outdated Show resolved Hide resolved
Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

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

Minor changes

@mikehardy
Copy link
Contributor Author

@Salakar down to 4 commits now, with a cleaner ignores list. Suggest handling global ignores issue as a separate item

@google-cla
Copy link

google-cla bot commented Dec 15, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@mikehardy
Copy link
Contributor Author

Okay, I rebased this to upstream/master and it seemed like for safety's sake I should include the commit that fixes firebase_ml_vision so it works with the final 25.x.x Android BoM (note that commit is battle-tested from react-native-firebase - you may check related links on it - it is ugly but correct)

I also fixed the issue where from a clean checkout the melos run qualitycheck works around the pub get/--no-pub issue where things aren't initialized fully after melos bootstrap so the whole thing really does work all the way through.

Unless auth rejects you for too many requests but that's out of my control 🤷

I'd love to see this or something like this integrated, it is the test harness I use to prove the firebase-ios-sdk v7+ / firebase-android-sdk v26+ #4249 work and auth emulator work #4263

Cheers :-)

@mikehardy
Copy link
Contributor Author

Until all the E2E workflow components that rely on network resources are wrapped in re-tries, the e2e jobs here will be subject to flakiness, it's not the PR that caused the firebase_messaging android failure, but I can't re-trigger that


[firebase_messaging_example]: Running Gradle task 'assembleDebug'...                          
[firebase_messaging_example]:   108.1s
[firebase_messaging_example]: 
[firebase_messaging_example]: 	at org.gradle.wrapper.Install.createDist(Install.java:48)
[firebase_messaging_example]: 	at org.gradle.wrapper.WrapperExecutor.execute(WrapperExecutor.java:128)
[firebase_messaging_example]: 	at org.gradle.wrapper.GradleWrapperMain.main(GradleWrapperMain.java:61)
[firebase_messaging_example]: 	Suppressed: java.net.SocketException: Broken pipe (Write failed)
[firebase_messaging_example]: 		at java.net.SocketOutputStream.socketWrite0(Native Method)
[firebase_messaging_example]: 		at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:111)
[firebase_messaging_example]: 		at java.net.SocketOutputStream.write(SocketOutputStream.java:155)
[firebase_messaging_example]: 		at sun.security.ssl.SSLSocketOutputRecord.encodeAlert(SSLSocketOutputRecord.java:81)
[firebase_messaging_example]: 		at sun.security.ssl.TransportContext.fatal(TransportContext.java:355)
[firebase_messaging_example]: 		at sun.security.ssl.TransportContext.fatal(TransportContext.java:267)
[firebase_messaging_example]: 		at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:397)
[firebase_messaging_example]: 		... 13 more
[firebase_messaging_example]: Caused by: java.io.EOFException: SSL peer shut down incorrectly
[firebase_messaging_example]: 	at sun.security.ssl.SSLSocketInputRecord.decode(SSLSocketInputRecord.java:167)
[firebase_messaging_example]: 	at sun.security.ssl.SSLTransport.decode(SSLTransport.java:108)
[firebase_messaging_example]: 	at sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1143)
[firebase_messaging_example]: 	... 15 more
[firebase_messaging_example]: Gradle task assembleDebug failed with exit code 1

also add parallelism to melos targets for full machine utilization
during local quality checks

- firebase_ml_custom currently has an intractable build error due to tflite plugin needing
TensorFlowLiteC 2.2.0 while up to date FirebaseMLVision needs 2.3.0

- firebase_admob and firebase_ml_vision do not have e2e tests for their examples
The intention is to easily return to a fresh-checkout status so that
developers can verify their environment will roughly match CI when it runs,
and thus expose any local problems

Incidentally, local example Podfiles are deleted here, as they are the source
of CI-versus-local-environment differences and ensuing CI failures
Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

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

LGTM!

LGTM GIF

@Salakar Salakar merged commit 024a11c into firebase:master Jan 4, 2021
@mikehardy mikehardy deleted the test-completeness branch January 4, 2021 19:01
@firebase firebase locked and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants