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

fix: Uploading performance metrics for multiple matrices #1329

Merged

Conversation

piotradamczyk5
Copy link
Contributor

@piotradamczyk5 piotradamczyk5 commented Nov 17, 2020

Fixes #1322

Test Plan

How do we know the code works?

  1. Run flank on Android physical device. For example with configuration entry:
gcloud:
...
  device:
    - model: OnePlus6T
      version: 28
...
  1. Run Flank with multiple matrices. Example with using option
...
flank:
  num-test-runs: 2
...
  1. Check that performanceMetrics.json is uploaded for each matrix

Checklist

  • Unit tested

@codecov-io
Copy link

codecov-io commented Nov 17, 2020

Codecov Report

Merging #1329 (8072aaa) into master (074cf0e) will decrease coverage by 0.04%.
The diff coverage is 78.78%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1329      +/-   ##
============================================
- Coverage     79.13%   79.09%   -0.05%     
+ Complexity      738      702      -36     
============================================
  Files           237      244       +7     
  Lines          4640     4683      +43     
  Branches        808      824      +16     
============================================
+ Hits           3672     3704      +32     
- Misses          550      551       +1     
- Partials        418      428      +10     

Copy link
Contributor

@pawelpasterz pawelpasterz left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@Sloox Sloox left a comment

Choose a reason for hiding this comment

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

Fix the tests and we all good!

@adamfilipow92 adamfilipow92 self-assigned this Nov 18, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 18, 2020

Timestamp: 2020-11-19 13:56:12
Buildscan url for ubuntu-workflow run 372369667
https://gradle.com/s/mr4dgjlgofh42

@adamfilipow92
Copy link
Contributor

https://github.com/Flank/flank/pull/1329/checks?check_run_id=1414353358

Tests fixed

@pawelpasterz
Copy link
Contributor

pawelpasterz commented Nov 18, 2020

I followed the test plan but can't find any performanceMetrics.json
Here is my config

gcloud:
  app: ~/app-debug.apk
  test: ~/app-single-success-debug-androidTest.apk
flank:
    num-test-runs: 3
    output-style: single

Let me know if I am doing something wrong

@piotradamczyk5
Copy link
Contributor Author

piotradamczyk5 commented Nov 18, 2020

I followed the test plan but can't find any performanceMetrics.json
Here is my config

gcloud:
  app: ~/app-debug.apk
  test: ~/app-single-success-debug-androidTest.apk
flank:
    num-test-runs: 3
    output-style: single

Let me know if I am doing something wrong

@pawelpasterz you must run on physical device. I have updated test plan

@pawelpasterz
Copy link
Contributor

I followed the test plan but can't find any performanceMetrics.json
Here is my config

gcloud:
  app: ~/app-debug.apk
  test: ~/app-single-success-debug-androidTest.apk
flank:
    num-test-runs: 3
    output-style: single

Let me know if I am doing something wrong

@pawelpasterz you must run on physical device. I have updated test plan

Thanks!

Copy link
Contributor

@pawelpasterz pawelpasterz left a comment

Choose a reason for hiding this comment

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

We have the option disable-results-upload. If set true we should not upload any results to the bucket. But I can see performanceMetrics.json are uploaded anyway
Screenshot 2020-11-18 at 15 04 16
With disable-results-upload set to false output is rather ugly
Screenshot 2020-11-18 at 15 12 47
I see some items we need to decide/discuss/fix

  1. Are performanceMetrics.json considered to be 'the same' results as the rest of flank's results?
  2. Let's align/beautify the output
  3. Let's decide what we want to print actually. Currently, we just print the message Uploading [file name] do we want to stick with that? Or we can print a gcs link but on a matrix level? So every matrix will have it's own. IMO we should decide on one convention and not mix them

Of course, I can be wrong. I'd like to get your opinion.

cc
@bootstraponline @jan-gogo

@piotradamczyk5
Copy link
Contributor Author

piotradamczyk5 commented Nov 18, 2020

We have the option disable-results-upload. If set true we should not upload any results to the bucket. But I can see performanceMetrics.json are uploaded anyway
Screenshot 2020-11-18 at 15 04 16
With disable-results-upload set to false output is rather ugly
Screenshot 2020-11-18 at 15 12 47
I see some items we need to decide/discuss/fix

  1. Are performanceMetrics.json considered to be 'the same' results as the rest of flank's results?
  2. Let's align/beautify the output
  3. Let's decide what we want to print actually. Currently, we just print the message Uploading [file name] do we want to stick with that? Or we can print a gcs link but on a matrix level? So every matrix will have it's own. IMO we should decide on one convention and not mix them

Of course, I can be wrong. I'd like to get your opinion.

cc
@bootstraponline @jan-gogo

I will disable uploading if disable-results-upload present
In the case of output, I agreed that I should not add a confirmation message (with the link), because it brokes our convention, I will delete it.
I will create the task to research and think about good output printing for uploading files

@bootstraponline bootstraponline force-pushed the #1322_fix_uploading_perfomance_metrics_for_multiple_matricies branch from cdefb25 to fb6a1e2 Compare November 18, 2020 16:03
@piotradamczyk5 piotradamczyk5 force-pushed the #1322_fix_uploading_perfomance_metrics_for_multiple_matricies branch from fb6a1e2 to d9565f7 Compare November 18, 2020 16:10
@bootstraponline bootstraponline force-pushed the #1322_fix_uploading_perfomance_metrics_for_multiple_matricies branch from 40e7c6f to b2ec447 Compare November 18, 2020 16:53
@piotradamczyk5 piotradamczyk5 dismissed pawelpasterz’s stale review November 19, 2020 07:18

I fixed all reported issues

@bootstraponline bootstraponline force-pushed the #1322_fix_uploading_perfomance_metrics_for_multiple_matricies branch from b2ec447 to 86c4585 Compare November 19, 2020 11:35
@bootstraponline bootstraponline force-pushed the #1322_fix_uploading_perfomance_metrics_for_multiple_matricies branch from 86c4585 to 8072aaa Compare November 19, 2020 13:37
@adamfilipow92
Copy link
Contributor

  • Tested without disable-results-upload works and output looks good
  • Tested with disable-results-upload works as expected

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.

Tested and works! 👍

@mergify mergify bot merged commit 6d836d1 into master Nov 20, 2020
@mergify mergify bot deleted the #1322_fix_uploading_perfomance_metrics_for_multiple_matricies branch November 20, 2020 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance metrics for num-test-runs
5 participants