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

Fix Android black box test connection locally #599

Merged
merged 1 commit into from
Jul 15, 2023
Merged

Fix Android black box test connection locally #599

merged 1 commit into from
Jul 15, 2023

Conversation

johnxwork
Copy link
Contributor

No description provided.

@johnxwork johnxwork force-pushed the android-bbt branch 2 times, most recently from d74ed56 to 150f544 Compare June 16, 2023 09:39
@johnxwork johnxwork changed the title Test Android bbt fix Fix Android black box test connection locally Jun 16, 2023
@johnxwork johnxwork force-pushed the android-bbt branch 5 times, most recently from b8fdbae to a93ff3e Compare June 19, 2023 05:46
oxve
oxve requested changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@oxve oxve left a comment

Choose a reason for hiding this comment

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

This PR includes a ton of formatting which makes it very hard to review. I'm not asking that you break out the formatting in this PR but to keep it in mind in the future.

Also, what is your formatter set to do? This looks like more than the standard pylint hook we run.

cobalt/black_box_tests/tests/evergreen_verify_qa_channel_update_test.py Outdated Show resolved Hide resolved
cobalt/black_box_tests/tests/evergreen_verify_qa_channel_update_test.py Outdated Show resolved Hide resolved
@johnxwork
Copy link
Contributor Author

This PR includes a ton of formatting which makes it very hard to review. I'm not asking that you break out the formatting in this PR but to keep it in mind in the future.

Also, what is your formatter set to do? This looks like more than the standard pylint hook we run.

I got the pre-commit hook wrong, so I used pyformat at some point. After that it became really difficult to revert :(

@johnxwork johnxwork force-pushed the android-bbt branch 6 times, most recently from 4b20add to 65d0410 Compare June 27, 2023 18:22
oxve
oxve reviewed Jun 27, 2023
View reviewed changes
cobalt/black_box_tests/threaded_web_server.py Outdated Show resolved Hide resolved
cobalt/black_box_tests/black_box_cobalt_runner.py Outdated Show resolved Hide resolved
cobalt/black_box_tests/black_box_cobalt_runner.py Outdated Show resolved Hide resolved
cobalt/black_box_tests/black_box_cobalt_runner.py Outdated Show resolved Hide resolved
cobalt/black_box_tests/proxy_server.py Outdated Show resolved Hide resolved
oxve
oxve approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@oxve oxve left a comment

Choose a reason for hiding this comment

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

Just a couple of small items left. PTAL and fix.

Otherwise I'm approving

cobalt/black_box_tests/black_box_cobalt_runner.py Outdated Show resolved Hide resolved
cobalt/black_box_tests/tests/preload_font.py Outdated Show resolved Hide resolved
cobalt/black_box_tests/threaded_web_server.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 27, 2023 •
edited
Loading

Codecov Report

Merging #599 (26e8754) into main (12db664) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
- Coverage   56.60%   56.57%   -0.03%     
==========================================
  Files        1884     1899      +15     
  Lines       93985    94415     +430     
==========================================
+ Hits        53196    53420     +224     
- Misses      40789    40995     +206

see 68 files with indirect coverage changes

TyHolc
TyHolc approved these changes Jun 27, 2023
View reviewed changes
cobalt/black_box_tests/black_box_cobalt_runner.py Outdated Show resolved Hide resolved
cobalt/black_box_tests/black_box_cobalt_runner.py Outdated Show resolved Hide resolved
starboard/tools/abstract_launcher.py Show resolved Hide resolved
@johnxwork johnxwork force-pushed the android-bbt branch 2 times, most recently from 67d9b05 to 9737923 Compare June 27, 2023 20:53
@kaidokert
Copy link
Member

I'll re-state again - i'm not really wanting that extensive change in all tests, there should be a less intrusive way to pipe the argument through without causing changes in all tests. Especially because the arg is specific to a single platform only.

@johnxwork
Proxy BBT fetch requests on Android using adb reverse
26e8754 
b/165629644

Change-Id: If0b7393a9582c7d68978add9a4d1e0a6505b437e
@johnxwork
Copy link
Contributor Author

wanting

Just removed all changes in python tests and turned to use a global variable for sending the port number.

@johnxwork johnxwork closed this Jul 14, 2023
@johnxwork johnxwork reopened this Jul 14, 2023
kaidokert
kaidokert approved these changes Jul 15, 2023
View reviewed changes
Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

This looks pretty clean to me - as far as this framework goes in any case.

If the tests run, lgtm

@johnxwork johnxwork merged commit 3e9e4e2 into youtube:main Jul 15, 2023
411 of 618 checks passed
niranjanyardi added a commit to niranjanyardi/cobalt that referenced this pull request Mar 15, 2024
@briantting @niranjanyardi
Update visual studio version to include llvm clang17 (youtube#599)
e11eb70 
Update visual studio version to include llvm clang17 - external

Change-Id: I4d7cefdec9879e78c9aae9acfa12bb715485deff

Co-authored-by: Niranjan Yardi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxve oxve oxve approved these changes

@kaidokert kaidokert kaidokert approved these changes

@TyHolc TyHolc TyHolc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johnxwork @kaidokert @oxve @TyHolc