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

test: Allow for the detection of whether to run Multiprocess tests with local clients or remote clients. #2054

Merged
merged 9 commits into from
Jul 12, 2022

Conversation

zain-mecklai
Copy link
Contributor

@zain-mecklai zain-mecklai commented Jul 6, 2022

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

  • No tests have been added.
  • No documentation changes or additions were necessary.

N/A for deprecated API

@zain-mecklai zain-mecklai changed the title Allow for the detection of whether to run locally or remotely. test: Allow for the detection of whether to run locally or remotely. Jul 6, 2022
}
}
else
else if (m_LaunchRemotely)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

@ashwinimurt
Copy link
Contributor

Couple of suggestions for current and future PRs:

  • We should be using the PR template, so that others looking at the PR will immediately know what the changes are, and whether it needs a changelog, what automated tests were run etc. Its perfectly fine to say n/a if its not applicable.
  • A good description would be nice to know that these are for Multiprocess tests. Since this is an external repo, more info in the description is always better.

@zain-mecklai zain-mecklai marked this pull request as ready for review July 7, 2022 13:52
@zain-mecklai zain-mecklai requested a review from a team as a code owner July 7, 2022 13:52
respond to PR feedback
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

LGTM!

@ashwinimurt ashwinimurt enabled auto-merge (squash) July 12, 2022 00:03
@zain-mecklai zain-mecklai changed the title test: Allow for the detection of whether to run locally or remotely. test: Allow for the detection of whether to run Multiprocess tests with local clients or remote clients. Jul 12, 2022
@ashwinimurt ashwinimurt merged commit ba30286 into develop Jul 12, 2022
@ashwinimurt ashwinimurt deleted the chore/multi-machine-phase-2 branch July 12, 2022 00:49
jakobbbb pushed a commit to GooseGirlGames/com.unity.netcode.gameobjects that referenced this pull request Feb 22, 2023
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants