-
Notifications
You must be signed in to change notification settings - Fork 57
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
e2e: Log errors #1683
Merged
Merged
e2e: Log errors #1683
+122
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
This comment was marked as outdated.
This comment was marked as outdated.
nirs
force-pushed
the
e2e-testing
branch
3 times, most recently
from
November 27, 2024 16:10
ea4e383
to
3f0f6d9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
nirs
force-pushed
the
e2e-testing
branch
4 times, most recently
from
November 29, 2024 00:02
e376f35
to
f53823a
Compare
nirs
commented
Nov 29, 2024
nirs
commented
Nov 29, 2024
nirs
force-pushed
the
e2e-testing
branch
2 times, most recently
from
December 4, 2024 13:55
2503c14
to
bc81805
Compare
This is an alternative to RamenDR#1596, trying to keep the test code more idiomatic, cleaner, and easier to use correctly. Add e2e/test.T type, embedding the standard library *testing.T, and keeping a logger. This type overrides testing.T methods logging to the standard library logger with methods logging to the specific logger. The Fatal[f] and Error[]f methods log an ERROR messages to make errors easy to find. To use the our T type, a test need to wrap the standard library *testing.T with the per-test logger: func TestFoo(dt *testing.T) { t := e2etesting.WithLog(dt, ctx.Logger()) Now we can use t transparently in this test: t.Log("This logs an INFO message to our logger") Note that the wrapped Error() and Fatal() method accept an error and will log a stracktrace: if err := ...; err != nil { // Log an error with a stracktrace and mark the test as failed. t.Error(err, "Failed, trying next step") } if err := ...; err != nil { // Log an error with a stracktrace and fail the test immediately. t.Fatal(err, "Failed, cannot continue") } Example error log (wrapped for readability): 2024-11-29T03:12:32.837+0200 ERROR subscr-deploy-rbd-busybox test/testing.go:55 Failed to failover workload: fake error in waitAndUpdateDRPC: failover to cluster "dr1" github.com/ramendr/ramen/e2e/test.(*T).Fatalf /Users/nsoffer/src/ramen/e2e/test/testing.go:55 github.com/ramendr/ramen/e2e/test.(*Context).Failover /Users/nsoffer/src/ramen/e2e/test/context.go:102 testing.tRunner /opt/homebrew/Cellar/go/1.23.3/libexec/src/testing/testing.go:1690 Notes: - All the test functions were updated to accept a `*testing.T` and wrap it with per-test log. Test helper functions accept now a `*e2e/test.T` so they don't need extra wrapping. - Sub tests requires wrapping the standard library t again, since testing.T is a type, not an interface. This adds one line of boilerplate for every sub test. - All Error[f] and Fatal[f] calls changed to format an error message with the underlying error so we should get more helpful error messages when something fails deep in the utility functions. - When DR sub test (e.g. Deploy) fails, we already logged the error by failing the sub test, so we call FailNow() in the parent test to abort the parent test silently. Issues: - The stracktrace does not include all callers leading the point the error was created, since fmt.Errorf() does not capture the stack. We can get better stacktrace by using another error package that does this like https://pkg.go.dev/github.com/go-errors/errors. - We need to disable the `thelper` linter in test.Context to allow using `dt *testing.T` and keep the test code idiomatic. Another way is to keep `t *testing.T`, and use `et` for the wrapped t. This makes the test code less idiomatic but it may be more clear that this is a non standard `t`. Fixes: RamenDR#1595 Signed-off-by: Nir Soffer <[email protected]>
raghavendra-talur
approved these changes
Dec 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an alternative to #1596, trying to keep the test code more
idiomatic, cleaner, and easier to use correctly.
Add e2e/test.T type, embedding the standard library *testing.T, and
keeping a logger. This type overrides testing.T methods logging to the
standard library logger with methods logging to the specific logger.
The Fatal[f] and Error[]f methods log an ERROR messages to make errors
easy to find.
To use the our T type, a test need to wrap the standard library
*testing.T with the per-test logger:
Now we can use t transparently in this test:
Note that the wrapped Error() and Fatal() method accept an error and
will log a stracktrace:
Example error log (wrapped for readability):
Notes:
All the test functions were updated to accept a
*testing.T
and wrapit with per-test log. Test helper functions accept now a
*e2e/test.T
so they don't need extra wrapping.
Sub tests requires wrapping the standard library t again, since
testing.T is a type, not an interface. This adds one line of
boilerplate for every sub test.
All Error[f] and Fatal[f] calls changed to format an error message
with the underlying error so we should get more helpful error messages
when something fails deep in the utility functions.
When DR sub test (e.g. Deploy) fails, we already logged the error by
failing the sub test, so we call FailNow() in the parent test to abort
the parent test silently.
Issues:
The stracktrace does not include all callers leading the point the
error was created, since fmt.Errorf() does not capture the stack. We
can get better stacktrace by using another error package that does
this like https://pkg.go.dev/github.com/go-errors/errors.
We need to disable the
thelper
linter in test.Context to allow usingdt *testing.T
and keep the test code idiomatic. Another way is tokeep
t *testing.T
, and useet
for the wrapped t. This makes thetest code less idiomatic but it may be more clear that this is a non
standard
t
.Fixes: #1595
Based on #1598 since we need to log on the right logger.