-
Notifications
You must be signed in to change notification settings - Fork 211
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
[Merged by Bors] - fix flaky test: TestSpacemeshApp_NodeService #4728
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4728 +/- ##
=========================================
+ Coverage 77.0% 77.2% +0.2%
=========================================
Files 255 255
Lines 28846 28855 +9
=========================================
+ Hits 22213 22286 +73
+ Misses 5250 5184 -66
- Partials 1383 1385 +2
|
03a28ef
to
f4ef0fb
Compare
bors try |
tryBuild failed: |
bors merge |
## Motivation Closes: #4729 ## Changes - The app during initialization for set the logging level to `zapcore.InfoLevel` which overwrites what ever logging configuration is passed in via `WithLog`. This was fixed, the app still starts up with `zapcore.InfoLevel` by default. - There has been an issue with logging in tests because the logging middleware for GRPC wasn't unset when the test ended which could result in components logging after the test ended and thereby failing a test. This is now done in `app.stopServices` (since it is set up in `app.startServices`) - After making logs visible the issue was identified: the node is shut down while it verifies its own post proof. This only happens on the `macos-latest` runner because other runners might not have completed their initial post proof yet or are already done verifying it by that time. - Changed verifying to only emit and log errors when an error other than `context.Cancelled` is returned. - These changes allowed me to run the test 100 times in a row without it failing a single time: ``` go test -v -timeout 150s -run ^TestSpacemeshApp_NodeService$ github.com/spacemeshos/go-spacemesh/node -count 100 ``` passes without a single iteration of the test failing. ## Test Plan n/a ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
Build failed: |
bors merge |
## Motivation Closes: #4729 ## Changes - The app during initialization for set the logging level to `zapcore.InfoLevel` which overwrites what ever logging configuration is passed in via `WithLog`. This was fixed, the app still starts up with `zapcore.InfoLevel` by default. - There has been an issue with logging in tests because the logging middleware for GRPC wasn't unset when the test ended which could result in components logging after the test ended and thereby failing a test. This is now done in `app.stopServices` (since it is set up in `app.startServices`) - After making logs visible the issue was identified: the node is shut down while it verifies its own post proof. This only happens on the `macos-latest` runner because other runners might not have completed their initial post proof yet or are already done verifying it by that time. - Changed verifying to only emit and log errors when an error other than `context.Cancelled` is returned. - These changes allowed me to run the test 100 times in a row without it failing a single time: ``` go test -v -timeout 150s -run ^TestSpacemeshApp_NodeService$ github.com/spacemeshos/go-spacemesh/node -count 100 ``` passes without a single iteration of the test failing. ## Test Plan n/a ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
Build failed:
|
c58c25c
to
3cefb86
Compare
bors merge |
## Motivation Closes: #4729 ## Changes - The app during initialization for set the logging level to `zapcore.InfoLevel` which overwrites what ever logging configuration is passed in via `WithLog`. This was fixed, the app still starts up with `zapcore.InfoLevel` by default. - There has been an issue with logging in tests because the logging middleware for GRPC wasn't unset when the test ended which could result in components logging after the test ended and thereby failing a test. This is now done in `app.stopServices` (since it is set up in `app.startServices`) - After making logs visible the issue was identified: the node is shut down while it verifies its own post proof. This only happens on the `macos-latest` runner because other runners might not have completed their initial post proof yet or are already done verifying it by that time. - Changed verifying to only emit and log errors when an error other than `context.Cancelled` is returned. - These changes allowed me to run the test 100 times in a row without it failing a single time: ``` go test -v -timeout 150s -run ^TestSpacemeshApp_NodeService$ github.com/spacemeshos/go-spacemesh/node -count 100 ``` passes without a single iteration of the test failing. ## Test Plan n/a ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
Pull request successfully merged into develop. Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
This reverts commit e91d735.
## Motivation #4728 broke debug logging, this reverts part of the PR to make it possible to log debug messages again when enabled via config. ## Changes Revert some changes from #4728 ## Test Plan n/a ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
## Motivation #4728 broke debug logging, this reverts part of the PR to make it possible to log debug messages again when enabled via config. ## Changes Revert some changes from #4728 ## Test Plan n/a ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
This reverts commit e91d735.
…shos#4728)"" This reverts commit f87546d.
Motivation
Closes: #4729
Changes
zapcore.InfoLevel
which overwrites what ever logging configuration is passed in viaWithLog
. This was fixed, the app still starts up withzapcore.InfoLevel
by default.app.stopServices
(since it is set up inapp.startServices
)macos-latest
runner because other runners might not have completed their initial post proof yet or are already done verifying it by that time.context.Cancelled
is returned.passes without a single iteration of the test failing.
Test Plan
n/a
TODO
DevOps Notes