Test updates (Enable race detection and disable output interceptor) #956

mikenairn · 2024-10-24T13:18:03Z

tests: Disable all output capture in ginkgo tests

This creates a lot of extra output during the test runs which is really distracting, and makes the output virtually useless.

Apart from that it can cause the entire test run to freeze under certain situations (which has happened a few times lately) due to the issue described here.
https://github.com/onsi/ginkgo/blob/104752fa2e97f04b1300754e710c419a63d2d30c/internal/output_interceptor.go#L13

tests: Add --race option to ginkgo run

This will cause the test suite to fail if any race condition is detected during the execution of the tests.

Note: When the tests are run in parallel this does not output a descriptive message for the failure. If it fails for seemingly random reasons you should try running the test suite without parallel processes enabled.

dev: Add --race option to make run task

Enables the race detector on locally running instances https://go.dev/doc/articles/race_detector. This will report useful
information in the logs about race related issues.

Example output https://gist.github.com/mikenairn/a165c9e8076f3c3437a066e8af5a9ce5

This creates a lot of extra output during the test runs which is really distracting, and makes the output virtually useless. Apart from that is can cause the entiore test run to freeze under certain situtations (which has happened a few times lately) due to the issue described here. https://github.com/onsi/ginkgo/blob/104752fa2e97f04b1300754e710c419a63d2d30c/internal/output_interceptor.go#L13 Signed-off-by: Michael Nairn <[email protected]>

mikenairn · 2024-10-24T13:32:49Z

Tests are likely going to fail because of a a race in the authorino tests:

WARNING: DATA RACE
Read at 0x00c00077eb40 by goroutine 542:
  runtime.mapaccess1_faststr()
      /home/mnairn/go/pkg/mod/golang.org/[email protected]/src/runtime/map_faststr.go:13 +0x0
  github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant.(*AffectedPolicyMap).IsPolicyAffected()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant/apimachinery_status_conditions.go:58 +0xfe
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).enforcedCondition()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_status.go:97 +0x6e
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).calculateStatus()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_status.go:78 +0x410
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).reconcileStatus()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_status.go:31 +0x1dc
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).Reconcile()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_controller.go:112 +0xc04
  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile()
      /home/mnairn/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:114 +0x1a1
  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler()
      /home/mnairn/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:311 +0x59a
  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem()
      /home/mnairn/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:261 +0x338
  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
      /home/mnairn/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:222 +0xb2

Previous write at 0x00c00077eb40 by goroutine 1402:
  runtime.mapdelete_faststr()
      /home/mnairn/go/pkg/mod/golang.org/[email protected]/src/runtime/map_faststr.go:301 +0x0
  github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant.(*AffectedPolicyMap).RemoveAffectedPolicy()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant/apimachinery_status_conditions.go:53 +0xed
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).desiredAuthConfig()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_authconfig.go:129 +0x84a
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).reconcileAuthConfigs()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_authconfig.go:30 +0xbb
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).reconcileResources()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_controller.go:185 +0x864
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).Reconcile()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_controller.go:109 +0xbc4
  github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).reconcileRouteParentGatewayPolicies.gowrap1()
      /home/mnairn/go/src/github.com/kuadrant/kuadrant-operator/controllers/authpolicy_controller.go:244 +0x8f

Probably the reason we are getting random failures on those tests

make/integration-tests.mk

eguzki · 2024-10-24T13:49:06Z

tests/common/authpolicy/suite_test.go

@@ -127,7 +127,6 @@ func TestMain(m *testing.M) {
 	logger := log.NewLogger(
 		log.SetLevel(log.DebugLevel),
 		log.SetMode(log.ModeDev),
-		log.WriteTo(GinkgoWriter),


Confused here. Writting to GinkgoWriter should be helpful as it only shows errors when the test fails and not when the test passes. That is desirable, isn't it??

Or maybe you want to have logs disabled entirely and only activate logs when I run one test?

I think this is an issue with ginkgo personally, or maybe i just don't know how to use it.

You are correct, anything written to GinkgoWriter shouldn't appear unless it fails, that is unless you use the -v option on ginkgo and then it will spit everything out regardless.

Problem is the output without -v is rubbish as it doesn't even tell you what tests it's running, just dots. What i would like is the output you get with -v, without the logs unless you get an error but i can't figure out any magic combination to make that happen.

This is the lowest priority thing in this PR btw, I'd happily drop that for now. The other changes are more important.

What i would like is the output you get with -v, without the logs unless you get an error

+1

Problem is the output without -v is rubbish as it doesn't even tell you what tests it's running

I do not see it as a problem. What is the problem there? We can make it optional, something that you can enable/disable.

Anyway. I am not blocking this in any way. Feel free to remove the configuration to write to GinkgoWriter.

LGTM

Being able to optionally log.WriteTo(GinkgoWriter) (default to false) would be nice, I guess. The otherwise noisy output sometimes helps us spot silent issues in advance, even when the tests succeed.

What about adding an env var to control it? (Willing to give up completely on the idea if you say it's too ugly of a solution.) We'd have to add it to the workflow_dispatch as a parameter too.

Yeah, this outputting happened since moving to the policy-machinery (previous to this, it worked as it should by outputting logs only when a test fails).

So this looks good to me until we resolve why the policy machinery is causing this

I have removed disabling the logs entirely from this PR, Will look at it separately, as i said it was the least important thing here.

I guess it's not the worst, i just like the more verbose output where it shows you the tests cases 🤷

Should still get the logs on failure this way i guess.

KevFan

Looks good to me 👍

This will cause the test suite to fail if any race condition is detected during the execution of the tests. Note: When the tests are run in parallel this does not output a descriptive message for the failure. If it fails for seemingly random reasons you should try running the test suite without parallel processes enabled. Signed-off-by: Michael Nairn <[email protected]>

Enables the race detector on locally running instances https://go.dev/doc/articles/race_detector. This will report useful information in the logs about race related issues. Signed-off-by: Michael Nairn <[email protected]>

Signed-off-by: Michael Nairn <[email protected]>

eguzki reviewed Oct 24, 2024

View reviewed changes

make/integration-tests.mk Outdated Show resolved Hide resolved

eguzki reviewed Oct 24, 2024

View reviewed changes

eguzki approved these changes Oct 24, 2024

View reviewed changes

mikenairn force-pushed the test_updates branch from 76d33f7 to 9062703 Compare October 24, 2024 14:46

KevFan approved these changes Oct 24, 2024

View reviewed changes

mikenairn added 3 commits October 24, 2024 15:50

dev: Add --race option to make run task

c42b19b

Enables the race detector on locally running instances https://go.dev/doc/articles/race_detector. This will report useful information in the logs about race related issues. Signed-off-by: Michael Nairn <[email protected]>

fix: Authorino race condition

521bd87

Signed-off-by: Michael Nairn <[email protected]>

mikenairn force-pushed the test_updates branch from 9062703 to 521bd87 Compare October 24, 2024 14:50

mikenairn changed the title ~~Test updates~~ Test updates (Enable race detection and disable output interceptor) Oct 24, 2024

mikenairn merged commit b6494ca into Kuadrant:main Oct 24, 2024
22 of 24 checks passed

mikenairn deleted the test_updates branch October 24, 2024 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test updates (Enable race detection and disable output interceptor) #956

Test updates (Enable race detection and disable output interceptor) #956

mikenairn commented Oct 24, 2024 •

edited

Loading

mikenairn commented Oct 24, 2024

eguzki Oct 24, 2024

mikenairn Oct 24, 2024

guicassolato Oct 24, 2024

eguzki Oct 24, 2024

guicassolato Oct 24, 2024

KevFan Oct 24, 2024

mikenairn Oct 24, 2024

mikenairn Oct 24, 2024

KevFan left a comment

Test updates (Enable race detection and disable output interceptor) #956

Test updates (Enable race detection and disable output interceptor) #956

Conversation

mikenairn commented Oct 24, 2024 • edited Loading

mikenairn commented Oct 24, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevFan left a comment

Choose a reason for hiding this comment

mikenairn commented Oct 24, 2024 •

edited

Loading