Migrate knative.dev/hack/shell to knative.dev/pkg/test/shell #2856

mgencur · 2023-10-10T10:59:15Z

Changes

Copy knative.dev/hack/shell to knative.dev/pkg/test/upgrade/shell and adjust tests which depend on specific paths.
Change shell.NewExecutor function to have this signature:

       func NewExecutor(t TestingT, loc ProjectLocation, opts... Option) Executor

Example usage can be seen in knative/serving#14495 . This way the logs from shell scripts are printed within test scope.

TODO: Mark the shell package in knative.dev/hack/shell as deprecated (via separate PR)

/kind

Fixes knative/serving#14461

knative/serving#14495 for knative/serving is also required to fully fix it.

Release Note

Docs

knative-prow · 2023-10-10T10:59:17Z

@mgencur: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

Changes

Copy knative.dev/hack/shell to knative.dev/pkg/test/shell and adjust tests which depend on specific paths.

Create testingWriter type which implements io.Writer and writes logs into testing.T.Logf Also provide a helper function TestingTStreams which can be used in other projects in this way:
       exec := shell.NewExecutor(shell.ExecutorConfig{
  	ProjectLocation: loc,
  	Streams:         shell.TestingTStreams(t),
  	Environ:         os.Environ(),
  })
Example usage can be seen in knative/serving#14495 . This way the logs from shell scripts are printed within test scope.

TODO: Mark the shell package in knative.dev/hack/shell as deprecated (via separate PR)

/kind

Fixes #

Release Note
Docs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-10-10T11:06:59Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (bae23eb) 81.83% compared to head (270ad20) 81.80%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
- Coverage   81.83%   81.80%   -0.03%     
==========================================
  Files         167      167              
  Lines       10227    10227              
==========================================
- Hits         8369     8366       -3     
- Misses       1612     1614       +2     
- Partials      246      247       +1

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cardil

Thanks, @mgencur! This looks promising.

There are a couple of places that require changes.

TLDR: We should force passing TestingT, and we can't fail in case of STDERR output.

cardil · 2023-10-10T14:30:55Z

test/shell/types.go

+	// If true, the test will be marked as failed if this testingWriter is
+	// ever used.
+	markFailed bool


This is no-go. The scripts often write messages to STDERR (in unix world usually debugging output goes to STDERR). The sole fact the script/tool writes something to STDERR doesn't mean we should fail the test.

cardil · 2023-10-10T14:34:39Z

test/shell/types.go

+
+// testingWriter writes to the given testing.TB.
+type testingWriter struct {
+	t testing.TB


Let's use our own interface. We just need one method:

type TestingT interface { Log(args ...any) }

OK. Just curious how does this help?

Helps with testing of functions, as they accept any object that suffices the interface.

This is just good practice in Golang. Here's example article on this https://www.thoughtworks.com/insights/blog/programming-languages/mistakes-to-avoid-when-coming-from-an-object-oriented-language

cardil · 2023-10-10T14:35:15Z

test/shell/executor.go

+
+// TestingTStreams returns Streams which writes to t.Log and marks
+// the test as failed if anything is written to Streams.Err.
+func TestingTStreams(t testing.TB) Streams {


Suggested change

func TestingTStreams(t testing.TB) Streams {

func testingTStreams(t TestingT) Streams {

cardil · 2023-10-10T14:38:40Z

test/shell/executor.go

+var ErrNoProjectLocation = errors.New("project location isn't provided")
+
+// NewExecutor creates a new executor from given config.
+func NewExecutor(config ExecutorConfig) Executor {


Suggested change

func NewExecutor(config ExecutorConfig) Executor {

func NewExecutor(t TestingT, config ExecutorConfig) Executor {

I would prefer to create an API that can't be misused. We probably want to force to pass the TestingT, we shouldn't allow people just switch without this and continue to output to os.Stdout, and os.Stderr.

The API shell.NewExecutor is general. It doesn't have to be used only in upgrade tests and doesn't always have to log to testing.T. Somebody can use it to log to normal stdout and stderr. That's why I keep it optional.
I will fix using this in a few repositories, at least Serving, Eventing, EventingKafkaBroker. I don't think it's a concern that somebody will migrate to the new package but will not use this new out/err. It's a narrow set of repositories. People need to know what they're doing, I think forcing this func NewExecutor(t TestingT, config ExecutorConfig) Executor { will just hide the fact. And it's also forcing users to pass testing.T even if they don't want to log to testing.T.

This shell executor is only allowed to be used in testing, see: knative/hack#32

And if so, it doesn't make sense to log to stdout/stderr in tests, as knative/serving#14461 clearly shows.

This is why I wanted a different API, fixing the project location, at the same time. I propose to have:

type Option func(*ExecutorConfig) func NewExecutor(t TestingT, loc ProjectLocation, opts... Option) Executor

Such API clearly says which params are required, and also allows customization if needed.

Wow. So this package is deprecated from the beginning? I think we're trying to do too many things with this fix. I like the previous API. From the example above it's not clear how ExecutorConfig is passed to the NewExecutor and it's increasing the num of parameters.
There would be further improvements needed:

actually move the "shell" package under test/pkg/upgrade because that's the only place where it can be used according to the validator code

remove "Streams" from the ExecutorConfig because it doesn't make sense anymore

Since this API is deprecated I don't see a point in improving it much. The goal is to fix the bug with logging.

There is a saying in 🇵🇱 - "Makeshift is the most durable."

The original PR was done in Nov, 2020 as a temporary solution. And it still is.

However, there is a way out of this mess. The Productivity WG is currently pushing this epic: knative/hack#254. In the process, at some point, it may be feasible to rewrite these install shell functions into proper Go-native packages. Those packages can be used directly in upgrade testing, so this package could be removed permanently, as intended.

But it will take time to get there.

So I think it's worth taking some extra steps to make it less buggy. Maybe people will start new projects and run into the same problem. If we change the API to not allow that, that would prevent those potential bugs.

I like both of your suggestions: moving the package to knative.dev/pkg/test/upgrade/shell and removing the Streams from ExecutorConfig.

OK. I pushed the changes. I left Streams within ExecutorConfig because it is the place where the stream configuration is held, it's later used by the executor.

cardil · 2023-10-10T14:39:55Z

test/shell/types.go

+// ExecutorConfig holds a executor configuration options.
+type ExecutorConfig struct {
+	ProjectLocation
+	Streams


Suggested change

Streams

* use own interface TestingT

It is now a required argument for NewExecutor function

cardil

/lgtm

It look

cardil

It looks great.

/lgtm

cardil

It looks great.

/lgtm

knative-prow · 2023-10-12T10:47:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, mgencur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [cardil]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cardil · 2023-10-12T10:48:46Z

Thanks @mgencur!

mgencur added 3 commits October 10, 2023 11:02

Move shell package from knative.dev/hack under test/shell

88698c0

Provide Streams for shell executor that writes to t.Log

974d18b

Remove temporary comment

3cfcbc6

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test-and-release labels Oct 10, 2023

knative-prow bot requested review from evankanderson and ReToCode October 10, 2023 10:59

Fix style

f6e0044

mgencur mentioned this pull request Oct 10, 2023

Shell executor logs through testing.T in upgrade tests knative/serving#14495

Merged

Fix lint

25f0adf

cardil requested changes Oct 10, 2023

View reviewed changes

knative-prow bot assigned cardil Oct 10, 2023

mgencur added 7 commits October 11, 2023 07:46

Do not fail when writing to stderr

64dda1e

* use own interface TestingT

Fix arguments for Logf

551d95b

Fix comments

7e238a1

NewExecutor function requires TestingT and ProjectLocation

ae89365

Move pkg/test/shell under pkg/test/upgrade/shell

b0c1be6

ProjectLocation is always passed

3abb743

It is now a required argument for NewExecutor function

Remove redundant file

270ad20

cardil approved these changes Oct 12, 2023

View reviewed changes

knative-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 12, 2023

cardil reviewed Oct 12, 2023

View reviewed changes

knative-prow bot merged commit 9051a45 into knative:main Oct 12, 2023
42 checks passed

cardil approved these changes Oct 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate knative.dev/hack/shell to knative.dev/pkg/test/shell #2856

Migrate knative.dev/hack/shell to knative.dev/pkg/test/shell #2856

mgencur commented Oct 10, 2023 •

edited

Loading

knative-prow bot commented Oct 10, 2023

Changes

codecov bot commented Oct 10, 2023 •

edited

Loading

cardil left a comment

cardil Oct 10, 2023

mgencur Oct 11, 2023

cardil Oct 10, 2023 •

edited

Loading

mgencur Oct 11, 2023

cardil Oct 12, 2023

cardil Oct 10, 2023

cardil Oct 10, 2023

mgencur Oct 11, 2023 •

edited

Loading

cardil Oct 11, 2023

mgencur Oct 11, 2023

cardil Oct 11, 2023

mgencur Oct 12, 2023

cardil Oct 10, 2023

cardil left a comment

cardil left a comment

cardil left a comment

knative-prow bot commented Oct 12, 2023

cardil commented Oct 12, 2023

	func TestingTStreams(t testing.TB) Streams {
	func testingTStreams(t TestingT) Streams {

	func NewExecutor(config ExecutorConfig) Executor {
	func NewExecutor(t TestingT, config ExecutorConfig) Executor {

Migrate knative.dev/hack/shell to knative.dev/pkg/test/shell #2856

Migrate knative.dev/hack/shell to knative.dev/pkg/test/shell #2856

Conversation

mgencur commented Oct 10, 2023 • edited Loading

Changes

knative-prow bot commented Oct 10, 2023

Changes

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

cardil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardil Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgencur Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardil left a comment

Choose a reason for hiding this comment

cardil left a comment

Choose a reason for hiding this comment

cardil left a comment

Choose a reason for hiding this comment

knative-prow bot commented Oct 12, 2023

cardil commented Oct 12, 2023

mgencur commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

cardil Oct 10, 2023 •

edited

Loading

mgencur Oct 11, 2023 •

edited

Loading