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

docs: Advise running ginkgo in verbose for e2e tests #15060

Merged

Conversation

pchaigno
Copy link
Member

We should always recommend to run end-to-end tests with ginkgo in verbose mode. Otherwise the provisioning may get stuck downloading some image without the contributor noticing.

We should always recommend to run end-to-end tests with ginkgo in verbose mode.
Otherwise the provisioning may get stuck downloading some image without the
contributor noticing.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno added release-note/misc This PR makes changes that have no direct user impact. sig/contributing Impacts contribution workflow, guidelines, and tools. labels Feb 22, 2021
@pchaigno pchaigno requested review from a team as code owners February 22, 2021 17:32
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 22, 2021
@@ -64,15 +64,15 @@ To run all of the runtime tests, execute the following command from the ``test``

::

ginkgo --focus="Runtime" -noColor
ginkgo -v --focus="Runtime" -noColor
Copy link
Member Author

Choose a reason for hiding this comment

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

Also wondering why we put -noColor everywhere. Such a shame to run in a black and white world...

Copy link
Member

Choose a reason for hiding this comment

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

Also wondering

Copy link
Contributor

Choose a reason for hiding this comment

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

I find -test.v useful also, did you consider adding it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know about -test.v. What's the difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's equivalent of go test -v, so if you have anything that uses test logger etc, it will be included. I don't know what it'd mean for Cilium tests, but I just use it to be sure when I run any gingko suite.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it makes any difference for Cilium tests, then yes, we should probably add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not so sure. Let's merge this and move along!

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good.
Should we make it verbose by default in the future, with an option to reduce log instead?

@qmonnet qmonnet removed their assignment Feb 22, 2021
@pchaigno pchaigno changed the title docs: Advice running ginkgo in verbose for e2e tests docs: Advise running ginkgo in verbose for e2e tests Feb 24, 2021
@pchaigno pchaigno added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Feb 26, 2021
@pchaigno
Copy link
Member Author

pchaigno commented Mar 1, 2021

Should we make it verbose by default in the future, with an option to reduce log instead?

@qmonnet It's a ginkgo option, so I'm not sure we can. If we can, then yes we should, absolutely.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 1, 2021
@qmonnet
Copy link
Member

qmonnet commented Mar 1, 2021

Maybe doable by accessing ginkgoconfig.DefaultReporterConfig.Verbose, but my knowledge of ginkgo is limited. Would need to give it a try.

@errordeveloper errordeveloper merged commit dd6acf8 into cilium:master Mar 2, 2021
1.10.0 automation moved this from In progress to Done Mar 2, 2021
@pchaigno pchaigno deleted the docs-verbose-ginkgo-e2e-tests branch March 2, 2021 16:02
@qmonnet
Copy link
Member

qmonnet commented Mar 3, 2021

Apparently I can get it verbose with the following patch:

Default to verbose
diff --git a/test/config/config.go b/test/config/config.go
index 17c7964aea76..1f05067b33ef 100644
--- a/test/config/config.go
+++ b/test/config/config.go
@@ -55,6 +55,7 @@ type CiliumTestConfigType struct {
 	// node. If false, some tests will silently skip multinode checks.
 	Multinode      bool
 	RunQuarantined bool
+	NoVerbose      bool
 	Help           bool
 }
 
@@ -107,6 +108,8 @@ func (c *CiliumTestConfigType) ParseFlags() {
 		"Enable tests across multiple nodes. If disabled, such tests may silently pass")
 	flagset.BoolVar(&c.RunQuarantined, "cilium.runQuarantined", false,
 		"Run tests that are under quarantine.")
+	flagset.BoolVar(&c.NoVerbose, "cilium.noVerbose", false,
+		"Disable verbose mode, otherwise enforced by default.")
 	flagset.BoolVar(&c.Help, "cilium.help", false, "Display this help message.")
 
 	args := make([]string, 0, len(os.Args))
diff --git a/test/ginkgo-ext/scopes.go b/test/ginkgo-ext/scopes.go
index 2253f764f17e..582a5b8bd770 100644
--- a/test/ginkgo-ext/scopes.go
+++ b/test/ginkgo-ext/scopes.go
@@ -120,6 +120,10 @@ func init() {
 	config.Flags(commandFlags, "ginkgo", true)
 	commandFlags.Parse(args)
 	ciliumTestConfig.CiliumTestConfig.ParseFlags()
+
+	if !ciliumTestConfig.CiliumTestConfig.NoVerbose {
+		config.DefaultReporterConfig.Verbose = true
+	}
 }
 
 func (s *scope) isUnset() bool {

I'll open a PR with this change so we can discuss it further. [Edit: #15184]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/contributing Impacts contribution workflow, guidelines, and tools.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants