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

mgencur · 2023-10-10T09:40:59Z

Fixes #14461

Depends on knative/pkg#2856

Temporarily depends on mgencur's fork of knative.dev/pkg. I will fix it once knative/pkg#2856 is merged.

Proposed Changes

Use the new shell.TestingTStreams(t) to define StdOut and StdErr for shell executor.
Pull dependencies from knative.dev/pkg

Release Note

codecov · 2023-10-10T09:47:57Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (eac93b0) 86.09% compared to head (5722efc) 86.05%.

❗ Current head 5722efc differs from pull request most recent head c12de81. Consider uploading reports for the commit c12de81 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14495      +/-   ##
==========================================
- Coverage   86.09%   86.05%   -0.05%     
==========================================
  Files         196      196              
  Lines       14887    14887              
==========================================
- Hits        12817    12811       -6     
- Misses       1759     1765       +6     
  Partials      311      311

see 4 files with indirect coverage changes

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

mgencur · 2023-10-10T10:00:05Z

This run has induced a failure in ServingHead script. The logs are printed within the test scope.

This run is successful without any failures. The logs are hidden as expected.

cardil · 2023-10-10T14:51:36Z

test/upgrade/installation/shell.go

 	loc, err := shell.NewProjectLocation("../../..")
 	if err != nil {
 		return err
 	}
 	exec := shell.NewExecutor(shell.ExecutorConfig{
 		ProjectLocation: loc,
+		Streams:         shell.TestingTStreams(t),


As I wrote in knative/pkg#2856 (comment), I think it's a bad idea to allow not to pass this custom streams. People may just replace deprecated package, and don't even know they should pass this.

How about the API look like:

exec := sheell.NewExecutor(t, loc, shell.ExecutorConfig{})

In Golang it's a good idea to default to an empty struct whenever it's possible.

The ExecutorConfig is a configuration object and I suppose it was created specifically to prevent passing too many arguments to NewExecutor (which it does nicely).
As I answered in the other PR, the custom streams are really "custom" , the executor doesn't need to log into testing.T if the user wants normal stdout/stderr. So if we want to keep this shell executor generic like it was before we should not enforce logging to testing.T.

cardil · 2023-10-12T15:38:41Z

vendor/knative.dev/pkg/test/upgrade/shell/executor.go

+	// Strip trailing newline because t.Log always adds one.
+	p = bytes.TrimRight(p, "\n")
+
+	w.t.Logf("%s", p)


It would be a good idea to Log every line of the output. Just for ascetic reasons. Now some lines are skewed in the output:

executor.go:207: Oct 12 07:08:25.738 install_head_reuse_ingress [OUT] *************************************** Oct 12 07:08:25.738 install_head_reuse_ingress [OUT] *** E2E TEST FAILED *** Oct 12 07:08:25.738 install_head_reuse_ingress [OUT] *** Start of information dump *** Oct 12 07:08:25.738 install_head_reuse_ingress [OUT] *************************************** executor.go:207: Oct 12 07:08:25.739 install_head_reuse_ingress [OUT] >>> The dump is located at /logs/artifacts/k8s.dump-.txt executor.go:207: Oct 12 07:08:26.048 install_head_reuse_ingress [OUT] >>> allowlistedv2workloads.auto.gke.io (0 objects)

See: https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/14495/upgrade-tests_serving_main/1712360626715627520

Something like:

Suggested change

w.t.Logf("%s", p)

for line := range strings.Split(p, "\n") {

w.t.Log(line)

}

It's a cosmetic change but why not. I've tested this in https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/14495/upgrade-tests_serving_main/1713811249294217216.
Anyway, this change would be in knative.dev/pkg so I can do it later. It doesn't affect this PR.

I've updated this PR and it's ready for review/merging.

Done in knative/pkg#2867

This reverts commit f5a5b50.

This reverts commit 52e151a.

This reverts commit bf60222.

dprotaso · 2023-10-16T14:56:25Z

/lgtm
/approve

thanks everyone - great improvement

knative-prow · 2023-10-16T14:56:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, 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:

~~OWNERS~~ [dprotaso]

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

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2023

knative-prow bot requested review from evankanderson and kauana October 10, 2023 09:41

knative-prow bot added area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Oct 10, 2023

mgencur mentioned this pull request Oct 10, 2023

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

Merged

cardil requested changes Oct 10, 2023

View reviewed changes

knative-prow bot assigned cardil Oct 10, 2023

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2023

mgencur force-pushed the shell_writing_to_testingt branch from f4127a3 to 9f4d95a Compare October 11, 2023 06:15

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 11, 2023

mgencur force-pushed the shell_writing_to_testingt branch from ee3ebd5 to f9b9908 Compare October 11, 2023 08:14

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2023

cardil reviewed Oct 12, 2023

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2023

mgencur added 11 commits October 16, 2023 08:52

Use shell.TestingTStreams in upgrade tests

fbd87aa

TMP: Use mgencurs fork of knative.dev/pkg

b42deea

Induce error

5f7ef74

Revert "Induce error"

1091c5c

This reverts commit f5a5b50.

Update deps

7ac81f3

Induce error

bf60222

Update deps

d443d7b

Update usage of shell.NewExecutor

fb060f5

Update deps

cd5a16e

Try printing each line separately

52e151a

Update deps

5722efc

mgencur force-pushed the shell_writing_to_testingt branch from 87d1a47 to 5722efc Compare October 16, 2023 06:56

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2023

mgencur added 2 commits October 16, 2023 09:26

Revert "Try printing each line separately"

f7f9b85

This reverts commit 52e151a.

Revert "Induce error"

c12de81

This reverts commit bf60222.

mgencur changed the title ~~[WIP] Shell executor logs through testing.T in upgrade tests~~ Shell executor logs through testing.T in upgrade tests Oct 16, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2023

knative-prow bot assigned dprotaso Oct 16, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2023

knative-prow bot merged commit 71085f8 into knative:main Oct 16, 2023
69 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mgencur commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

mgencur commented Oct 10, 2023 •

edited

Loading

cardil Oct 10, 2023 •

edited

Loading

mgencur Oct 11, 2023

cardil Oct 12, 2023

mgencur Oct 16, 2023

mgencur Oct 16, 2023 •

edited

Loading

mgencur Oct 16, 2023

dprotaso commented Oct 16, 2023

knative-prow bot commented Oct 16, 2023

-	w.t.Logf("%s", p)
+	for line := range strings.Split(p, "\n") {
+		w.t.Log(line)
+	}

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

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

Conversation

mgencur commented Oct 10, 2023 • edited Loading

Proposed Changes

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

mgencur commented Oct 10, 2023 • edited Loading

cardil Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

mgencur Oct 11, 2023

Choose a reason for hiding this comment

cardil Oct 12, 2023

Choose a reason for hiding this comment

mgencur Oct 16, 2023

Choose a reason for hiding this comment

mgencur Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

mgencur Oct 16, 2023

Choose a reason for hiding this comment

dprotaso commented Oct 16, 2023

knative-prow bot commented Oct 16, 2023

mgencur commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

mgencur commented Oct 10, 2023 •

edited

Loading

cardil Oct 10, 2023 •

edited

Loading

mgencur Oct 16, 2023 •

edited

Loading