Fuchsia-on-Clusterfuzz integration test.#398
Conversation
Verifies that the Fuchsia launcher can run Fuchsia on QEMU and SSH into it accordingly. Also does some minor fixups throughout the Fuchsia code (does QEMU teardown after fuzzing is finished, ensures portnum is a str and not an int, etc)
| # case, with BaseLauncherTest as the base) | ||
| # TODO(flowerhack): Does it make more sense to factor out all the setUp | ||
| # into its own method, which will have to be specifically called by every | ||
| # class in this file? |
There was a problem hiding this comment.
This is a question for y'all; I'm neutral either way, since neither route is super-aesthetically-pleasing for me
There was a problem hiding this comment.
I'm confused, why is there a need to call setUp manually?
If you want to share it across multiple classes maybe use a mixin?
There was a problem hiding this comment.
Right, maybe my phrasing wasn't super clear:
-
the with_cloud_emulators wrapper wraps a class, in this case TestLauncherFuchsia, and calls its setUp method
-
but, it does not call any further parent's setUp methods! (e.g. BaseLauncherTest, which is what we want)
-
and, if you try to call super(self).setUp within TestLauncherFuchsia, it winds up recursing infinitely, since you're just calling the decorator's setUp method
Do you prefer a mixin to simply being explicit when calling setUp while using this decorator?
(I moved this line into setUp but it's still the same line fyi)
|
requesting review from @oliverchang and/or @jonathanmetzman (and add others if you feel that's more appropriate!) |
|
/gcbrun |
1 similar comment
|
/gcbrun |
|
|
||
| environment.set_value('QUEUE_OVERRIDE', 'FUCHSIA') | ||
| environment.set_value('OS_OVERRIDE', 'FUCHSIA') | ||
| os.environ['FUCHSIA_RESOURCES_URL'] = 'gs://fuchsia-on-clusterfuzz-v2/*' |
There was a problem hiding this comment.
What happened to v1?
There was a problem hiding this comment.
this just happened to be the name of the cloud bucket i created for this project; v1 was more of a "testing ground" in practice. i could rename the bucket to something else, if desired, but it's really just a UUID for where we're storing a copy of the SDK & friends for now.
| # case, with BaseLauncherTest as the base) | ||
| # TODO(flowerhack): Does it make more sense to factor out all the setUp | ||
| # into its own method, which will have to be specifically called by every | ||
| # class in this file? |
There was a problem hiding this comment.
I'm confused, why is there a need to call setUp manually?
If you want to share it across multiple classes maybe use a mixin?
|
/gcbrun |
|
/gcbrun |
|
@jonathanmetzman think i've responded to everything for now... |
oliverchang
left a comment
oliverchang
left a comment
There was a problem hiding this comment.
nice! looks mostly good. just some very minor comments.
| if timeout_multiplier > 1: | ||
| testcase_count /= timeout_multiplier | ||
|
|
||
| qemu_process = None |
There was a problem hiding this comment.
nit: this line is unnecessary. variables set in the if have function scope.
There was a problem hiding this comment.
but we need access to qemu_process again on L1482, right?
There was a problem hiding this comment.
yeah, but it should still work, because it's only ever accessed when platform == 'FUCHSIA'
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
4 participants
Verifies that the Fuchsia launcher can run Fuchsia on QEMU and SSH into
it accordingly.
Also does some minor fixups throughout the Fuchsia code (does QEMU
teardown after fuzzing is finished, ensures portnum is a str and not an
int, etc)
A few specific questions for reviewers are asked inline.