Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: allow custom port in samples tests #272

Conversation

DominicKramer
Copy link
Contributor

This allows multiple runs of the samples tests to occur on the
same machine at the same time to test for conflicts between
concurrently running tests.

This allows multiple runs of the samples tests to occur on the
same machine at the same time to test for conflicts between
concurrently running tests.
@DominicKramer DominicKramer requested a review from a team March 10, 2019 19:37
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 10, 2019
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

How will this actually help us in the tests? Seems like it would be better to have the quickstart main method accept the port as a command line arg, then choose a high order random port in the test to pass to the sample. I don't think we want to use an env var here.

@codecov
Copy link

codecov bot commented Mar 10, 2019

Codecov Report

Merging #272 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #272   +/-   ##
=======================================
  Coverage   94.87%   94.87%           
=======================================
  Files           2        2           
  Lines          78       78           
  Branches        8        8           
=======================================
  Hits           74       74           
  Misses          4        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f60e41f...e63bc50. Read the comment docs.

@kjin
Copy link
Contributor

kjin commented Mar 11, 2019

@JustinBeckwith For the record, I think this is a pretty standard thing to do that I've seen in other samples (using env var).

@DominicKramer
Copy link
Contributor Author

@JustinBeckwith I'm fine using an env var or as a program argument. This PR itself doesn't fix any issues in the test. However, when trying them locally it is nice to have multiple instance running concurrently to test for issues. Having the port adjustable makes that easier.

Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

Got it. If the goal is to make the tests more reliable, this isn't gonna help. But if the goal was just allow setting the port locally, LGTM.

@DominicKramer DominicKramer merged commit ab44701 into googleapis:master Mar 11, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants