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

[WIP] [CI] new command implementation for tests #4040

Closed
wants to merge 6 commits into from

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Mar 27, 2025

Note this is a large PR, though the bulk of the changes are under a couple of files in mod/tigron/internal.

This PR is a tigron changeset addressing a few important issues:

In a shell: icmd does not provide value for us. Most of its features we do not use, and the simple wrapping around golang command exec is causing more issues than it solves - icmd does replace the process standard streams with its own buffers, which gets in the way of using ptys, does not provide proper timeout logic for stuck processes, and does not seem to kill process groups properly.

This PR does away with it, and replace it with an internal com.Command that does offer:

  • proper timeout
  • pty handling
  • proper process group termination

... along with absorbing a large part of the logic that was previously baked into test/command.go (env filtering, cloning, command wrapping).

While this should technically be (mostly) transparent for tests, some of the logic changes with timeout and pty / stdin handling required adaptation of some functions signatures.

Specifically:

  • Background(timeout) is now simply Background(), as the new WithTimeout helper now applies to any command, and not just the background-ed ones
  • WithPTY no longer supports the extra "writers" argument. To write to stdin, the same methods can now be used whether or not we are working with a PTY
  • the previous WithStdin method was ill-named/ill-advised, as it is not setting stdin, but writing to stdin, can be used repeatedly, and lacked flexibility. It has been replaced by Feed(io.Reader) for the simpler cases, and WithFeeder(func()io.Reader) that allows for more complex logic before writing to stdin (like Pty(writers) used to)

Finally, this PR also provides stuff that came up as necessary while working on the new command:
- minimal improvements over debugging information output readability
- (cosmetic) changes to prepare for version 2 of golangci
- the testing of internal Command itself did motivate the addition of leak detection and an internal assert library

While on the surface this PR is minimally impactful, this is actually a profound change, and I assume a few rounds will be needed before taking it out of "draft". Specifically, the current default timeout for commands execution is set at 3 minutes, and we might want to adjust that (eg: shorten it generally, and set it explicitly for tests that require longer)

Blocked by:

Preferable to merge after:

TODO:

  • green CI
  • mute the logging from pipes by default

@apostasie apostasie force-pushed the ci-tigron-cmd branch 3 times, most recently from 67c8b3f to 7f7792e Compare March 27, 2025 04:16
@apostasie apostasie changed the title [CI] new command implementation for tests [WIP] [CI] new command implementation for tests Mar 27, 2025
@apostasie
Copy link
Contributor Author

@AkihiroSuda thanks for the early review on this.

I am breaking it down in smaller chunks to simplify the discussion, so, let see what remains here once we are done with the more trivial stuff in the breakout PRs.

@apostasie apostasie force-pushed the ci-tigron-cmd branch 4 times, most recently from 5870861 to 1bd3bf8 Compare March 27, 2025 21:53
@@ -21,9 +21,12 @@ const (
ExitCodeSuccess = 0
// ExitCodeGenericFail will verify that the command ran and exited with a non-zero error code.
// This does NOT include timeouts, cancellation, or signals.
ExitCodeGenericFail = -1
ExitCodeGenericFail = -10
Copy link
Contributor Author

@apostasie apostasie Mar 28, 2025

Choose a reason for hiding this comment

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

Note: in some cases, go exec may return -1 exit code, which would be colliding with these in case someone wanted to test that specific condition.

@apostasie apostasie force-pushed the ci-tigron-cmd branch 7 times, most recently from 1a9d20f to ece2925 Compare March 29, 2025 16:16
Signed-off-by: apostasie <[email protected]>
@apostasie apostasie closed this Mar 29, 2025
@apostasie apostasie deleted the ci-tigron-cmd branch March 29, 2025 17:38
@apostasie
Copy link
Contributor Author

Geeeeeeeez github.

@AkihiroSuda moving to #4056

Sorry about that.

Done:

  • yanked out the logger
  • CI is (should be) green
  • cleaned-up the commits

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.

2 participants