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

Add a commented-out CopySuite.TestRunShell #320

Merged
merged 1 commit into from
Mar 27, 2017

Conversation

mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Mar 24, 2017

We are maintaining code to set up and run registries, including the fairly complex setup for Atomic Registry, in the integration tests. This is all useful for experimentation in shell, and the easiest way to
do that is to add a “test” which, after all the set up is done, simply starts a shell.

Obviously this is commented out and thus does not affect ordinary test runs.

A possible alternative would be to convert all of the setup code not to depend on check.C and testing.T, but that would be fairly cumbersome due to how prevalent c.Logf and c.Assert are throughout the setup code. Especially the natural replacement of c.Assert with a panic() would be
pretty ugly, and adding real error handling to all of that would make the code noticeably longer. Commenting/uncommenting a function and copy&pasting a command works just as well, at least for now.

(It is not conveniently possible to create a new “main program” which manually creates a check.C and testing.T just for the purpose of running the setup code either; check.C can be created given a testing.T, but testing.T is only created by testing.MainStart, which does not allow us to submit a non-test method and testing.MainStart is excluded from the Go compatibility promise.)

TestRunShell is not really a test; it is a convenient way to use the registry setup code
in openshift.go and CopySuite to get an interactive environment for experimentation.

To use it, uncomment the “test” below, and run:
Copy link
Member

Choose a reason for hiding this comment

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

should "test" be "the TestRunShell() function"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be; but really commenting out the code is pretty cumbersome (we also have to comment out an import), so I have rewritten this to use a build tag instead.

We are maintaining code to set up and run registries, including the
fairly complex setup for Atomic Registry, in the integration tests.
This is all useful for experimentation in shell, and the easiest way to
do that is to add a “test” which, after all the set up is done, simply
starts a shell.

This is gated by a build tag, so it does not affect normal test runs.

A possible alternative would be to convert all of the setup code not to
depend on check.C and testing.T, but that would be fairly cumbersome due
to how prevalent c.Logf and c.Assert are throughout the setup code.
Especially the natural replacement of c.Assert with a panic() would be
pretty ugly, and adding real error handling to all of that would make
the code noticeably longer.  The build tag and copy&pasting a command
works just as well, at least for now.

(It is not conveniently possible to create a new “main program” which
manually creates a check.C and testing.T just for the purpose of running
the setup code either; check.C can be created given a testing.T, but
testing.T is only created by testing.MainStart, which does not allow us
to submit a non-test method; and testing.MainStart is excluded from the
Go compatibility promise.)
@runcom
Copy link
Member

runcom commented Mar 27, 2017

LGTM

@mtrmac
Copy link
Contributor Author

mtrmac commented Mar 27, 2017

Modified this to use a build tag instead asking the user to manually uncomment a function. That should be easier to both use and maintain.

(Alternatively, we could use c.Skip in a test, but that would clutter the output by default.)

@mtrmac
Copy link
Contributor Author

mtrmac commented Mar 27, 2017

@runcom Sorry, PTAL at the new version.

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the 411 and the rework.

@runcom
Copy link
Member

runcom commented Mar 27, 2017

@mtrmac build tag looks good to me as well.

@mtrmac mtrmac merged commit 0734c4c into containers:master Mar 27, 2017
@mtrmac mtrmac deleted the openshift-shell branch March 27, 2017 15:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants