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.

Sign up for GitHub

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

Jump to bottom

feat: upload matrix ids #1393

Merged
merged 6 commits into from
Dec 14, 2020
Merged

feat: upload matrix ids #1393

merged 6 commits into from
Dec 14, 2020

Conversation

Sloox
Copy link
Contributor

@Sloox Sloox commented Dec 10, 2020 •
edited by jan-goral
Loading

Fixes #1381

Test Plan

How do we know the code works?

  1. Run any flank configuration which will not fail before executing test matrices.
  2. Wait until execution finish.
  3. Verify the matrices_ids.json is uploaded to the bucket.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 10, 2020 •
edited
Loading

Timestamp: 2020-12-14 09:27:01
Buildscan url for ubuntu-workflow run 420388477
https://gradle.com/s/nwvgpctkkfg2i

@codecov-io
Copy link

codecov-io commented Dec 10, 2020 •
edited
Loading

Codecov Report

Merging #1393 (3a88895) into master (7f9fdcf) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1393      +/-   ##
============================================
+ Coverage     77.61%   77.70%   +0.09%     
- Complexity      717      719       +2     
============================================
  Files           248      248              
  Lines          4788     4794       +6     
  Branches        915      915              
============================================
+ Hits           3716     3725       +9     
+ Misses          572      571       -1     
+ Partials        500      498       -2

@Sloox Sloox requested a review from jan-goral December 10, 2020 13:44
@Sloox Sloox marked this pull request as ready for review December 10, 2020 13:44
@pawelpasterz
Copy link
Contributor

Please, remember to update the integration tests

@adamfilipow92
Copy link
Contributor

I know we have always in the ticket title but, what when we set disable-results-upload? In this case, we not uploading any report or _shards.json maybe this file shouldn't be uploaded too?

@pawelpasterz
Copy link
Contributor

I know we have always in the ticket title but, what when we set disable-results-upload? In this case, we not uploading any report or _shards.json maybe this file shouldn't be uploaded too?

Actually good point!

@jan-goral
Copy link
Contributor

jan-goral commented Dec 11, 2020 •
edited
Loading

I know we have always in the ticket title but, what when we set disable-results-upload? In this case, we not uploading any report or _shards.json maybe this file shouldn't be uploaded too?

I have added fix to my suggestion #1393 (comment)

@Sloox Sloox requested a review from jan-goral December 11, 2020 12:14
@Sloox
Copy link
Contributor Author

Sloox commented Dec 11, 2020

@pawelpasterz can you please verify the integration test yourside. to see if anything fails. I cannot even run them at all on 3 different machines (Windows/ Mac or ubuntu)

Sloox
Sloox commented Dec 11, 2020
View reviewed changes
integration_tests/src/test/resources/compare/MultipleApksIT-compare
AndroidArgs gcloud: results-bucket: test-lab-[a-zA-Z0-9-]* results-dir: [.a-zA-Z0-9_-]* record-video: false timeout: 15m async: false client-details: network-profile: null results-history-name: null # Android gcloud app: [0-9a-zA-Z\\\/_.:-]*[\\\/]test_runner[\\\/]src[\\\/]test[\\\/]kotlin[\\\/]ftl[\\\/]fixtures[\\\/]tmp[\\\/]apk[\\\/]app-debug.apk test: null additional-apks: auto-google-login: false use-orchestrator: false directories-to-pull: grant-permissions: all type: null other-files: scenario-numbers: scenario-labels: obb-files: obb-names: performance-metrics: false num-uniform-shards: null test-runner-class: null test-targets: robo-directives: robo-script: [0-9a-zA-Z\\\/_.:-]*[\\\/]test_runner[\\\/]src[\\\/]test[\\\/]kotlin[\\\/]ftl[\\\/]fixtures[\\\/]tmp[\\\/]apk[\\\/]MainActivity_robo_script.json device: - model: NexusLowRes version: 28 locale: en orientation: portrait num-flaky-test-attempts: 0 test-targets-for-shard: fail-fast: false\s* flank: max-test-shards: 50 shard-time: -1 num-test-runs: 1 smart-flank-gcs-path:\s smart-flank-disable-upload: false default-test-time: 120.0 use-average-test-time-for-new-tests: false files-to-download: test-targets-always-run: disable-sharding: false project: flank-open-source local-result-dir: results full-junit-result: false # Android Flank Yml keep-file-path: false additional-app-test-apks: - app: null test: [0-9a-zA-Z\\\/_.:-]*[\\\/]test_runner[\\\/]src[\\\/]test[\\\/]kotlin[\\\/]ftl[\\\/]fixtures[\\\/]tmp[\\\/]apk[\\\/]app-multiple-success-debug-androidTest.apk - app: null test: [0-9a-zA-Z\\\/_.:-]*[\\\/]test_runner[\\\/]src[\\\/]test[\\\/]kotlin[\\\/]ftl[\\\/]fixtures[\\\/]tmp[\\\/]apk[\\\/]app-multiple-error-debug-androidTest.apk - app: null test: gs:\/\/flank-open-source.appspot.com\/integration\/app-single-success-debug-androidTest.apk run-timeout: -1 legacy-junit-result: false ignore-failed-tests: false output-style: single disable-results-upload: false default-class-test-time: 240.0\s*RunTests\s* Smart Flank cache hit: 0\% \(0 \/ 9\) Shard times: 120s, 120s, 120s, 120s, 120s, 240s, 240s, 240s, 240s\s* Smart Flank cache hit: 0\% \(0 \/ 9\) Shard times: 120s, 120s, 120s, 120s, 120s, 240s, 240s, 240s, 240s\s* Smart Flank cache hit: 0\% \(0 \/ 1\) Shard times: 120s\s*Saved 3 shards to android_shards.json Uploading android_shards.json \.* Uploading app-debug.apk \.*[\s\S]* 11 tests \+ 8 parameterized classes \/ 19 shards\s* 4 matrix ids created in \d{1,2}m \d{1,2}s https:\/\/console.developers.google.com\/storage\/browser\/test-lab-[a-zA-Z0-9_-]*\/[.a-zA-Z0-9_-]*\s*Matrices webLink matrix-[a-zA-Z0-9]* https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)? matrix-[a-zA-Z0-9]* https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)? matrix-[a-zA-Z0-9]* https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)? matrix-[a-zA-Z0-9]* https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)?[\s\S]*CostReport Virtual devices \$\d{1,2}.\d{1,2} for \d{1,2}m[\s\S]* Uploading CostReport.txt \.*MatrixResultsReport 3 \/ 4 \(75\.00\%\) 1 matrices failed[\s\S]*More details are available at:https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)?\s* Uploading MatrixResultsReport.txt \.* Uploading HtmlErrorReport.html \.* Uploading JUnitReport.xml \.* Uploading matrix_ids.json \.*FetchArtifacts \.* Updating matrix file\s*Matrices webLink matrix-[a-zA-Z0-9]* https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)? matrix-[a-zA-Z0-9]* https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)? matrix-[a-zA-Z0-9]* https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)? matrix-[a-zA-Z0-9]* https:\/\/console.firebase.google.com\/project\/flank-open-source\/testlab\/histories\/[.a-zA-Z0-9_-]*\/matrices\/[.a-zA-Z0-9_-]*(\/executions\/[.a-zA-Z0-9_-]*)?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamfilipow92 thanks. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem 😄

adamfilipow92
adamfilipow92 approved these changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@adamfilipow92 adamfilipow92 left a comment

Choose a reason for hiding this comment

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

  • Code looks good
  • IT updated
    Great work! 👍

@bootstraponline bootstraponline force-pushed the #1381-upload-matrix-ids-json branch from e7eb898 to fc40ea9 Compare December 11, 2020 15:50
piotradamczyk5
piotradamczyk5 reviewed Dec 11, 2020
View reviewed changes
test_runner/src/main/kotlin/ftl/gc/GcStorage.kt
rootGcsBucket = args.resultsBucket,
runGcsPath = args.resultsDir
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add unit test to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually @jan-gogo and I were chatting about it. We decided against it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, but still, we can decide to add if someone decides that unit test for it is important.

@bootstraponline bootstraponline force-pushed the #1381-upload-matrix-ids-json branch 3 times, most recently from 32e0181 to 5e6c296 Compare December 14, 2020 07:48
@bootstraponline bootstraponline force-pushed the #1381-upload-matrix-ids-json branch from 5e6c296 to 3a88895 Compare December 14, 2020 09:22
jan-goral
jan-goral reviewed Dec 14, 2020
View reviewed changes
Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

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

The implementation looks well, but I need to verify if a matrix_ids.json is uploaded to the bucket. Currently, Google servers are down.

@jan-goral
Copy link
Contributor

jan-goral commented Dec 14, 2020 •
edited
Loading 

* [x]  Code looks good

* [x]  IT updated
  Great work! 👍

@adamfilipow92 Next time attach links to the results so anyone could verify them.

jan-goral
jan-goral approved these changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

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

looks ok:

@mergify mergify bot merged commit 2b1dbb1 into master Dec 14, 2020
@mergify mergify bot deleted the #1381-upload-matrix-ids-json branch December 14, 2020 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piotradamczyk5 piotradamczyk5 piotradamczyk5 left review comments

@adamfilipow92 adamfilipow92 adamfilipow92 approved these changes

@jan-goral jan-goral jan-goral approved these changes

@bootstraponline bootstraponline Awaiting requested review from bootstraponline

@pawelpasterz pawelpasterz Awaiting requested review from pawelpasterz

@zuziaka zuziaka Awaiting requested review from zuziaka

Assignees

@Sloox Sloox

@jan-goral jan-goral

Labels
Feature Tiger 🐯
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Always upload matrix_ids.json to GCS
6 participants
@Sloox @codecov-io @pawelpasterz @adamfilipow92 @jan-goral @piotradamczyk5