-
Notifications
You must be signed in to change notification settings - Fork 441
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: Allow for the detection of whether to run Multiprocess tests with local clients or remote clients. #2054
Conversation
testproject/Assets/Tests/Runtime/MultiprocessRuntime/BaseMultiprocessTests.cs
Outdated
Show resolved
Hide resolved
} | ||
} | ||
else | ||
else if (m_LaunchRemotely) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why else if? Why not just else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually prefer to be explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it's an if on a single bool, that's pretty explicit already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fair, I'll change it.
testproject/Assets/Tests/Runtime/MultiprocessRuntime/BaseMultiprocessTests.cs
Show resolved
Hide resolved
Couple of suggestions for current and future PRs:
|
respond to PR feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…nity-Technologies#2054) * Set up for launching remotely * Set up for launching remotely * Report on WorkerCount and LaunchRemotely state * Store the Process as it will complete later * add a little more logging * Simplified name * Update GetWorkerCount to no longer set internal state * Update BaseMultiprocessTests.cs respond to PR feedback Co-authored-by: ashwini <36935028+ashwinimurt@users.noreply.github.com>
Added dependency on the environment variable
MP_PLATFORM_LIST
. Based on the existence of this variable the Multiprocess tests will switch between running clients locally and running them on remote nodes.Add parsing code for using the value of the variable to determine which types of hosts to run the clients on.
No Jira associated with this whole project.
No RFC associated with this whole project
Changelog
No changes for the testlog
Testing and Documentation
N/A for deprecated API