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

Reduce console noise coming from unit tests: #4166

Closed
wants to merge 2 commits into from

Conversation

scottschurr
Copy link
Collaborator

Context of Change

A few unit tests have historically generated noise to the console caused by log writes. This noise was not useful and made it harder to locate actual test failures.

By changing the log level of these tests from

  • severities::kError to
  • severities::kDisabled

it was possible to remove that noise coming from the logs.

Only those unit tests currently producing log noise were changed. This is so, in the future, we can see if pre-existing unit tests start producing unexpected log noise.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

A few unit tests have historically generated a lot of noise
to the console from log writes.  This noise was not useful
and made it harder to locate actual test failures.

By changing the log level of these tests from
- severities::kError to
- severities::kDisabled
it was possible to remove that noise coming from the logs.
@scottschurr scottschurr added the Tech Debt Non-urgent improvements label May 16, 2022
@scottschurr scottschurr requested review from ximinez and undertome May 16, 2022 17:07
@undertome
Copy link
Contributor

I noticed about 135 lines fewer with your change, but I still picked up on some noise from ValidatorSite tests:

ripple.app.ValidatorSite Fetch list - /redirect_forever/301 [https] v1 Exceeded max redirects
-- Msg: max redirects
ripple.app.ValidatorSite Fetch list - /redirect_forever/307 [https] v1 Exceeded max redirects, /redirect_forever/308 [https] v1 Exceeded max redirects
-- Msg: max redirects
-- Msg: max redirects
ripple.app.ValidatorSite Fetch list - /validators [https] v1 , /redirect_forever/302 [https] v1 Exceeded max redirects
-- Msg: max redirects
ripple.app.ValidatorSite Fetch list - /validators2 [https] v1 , /redirect_forever/302 [https] v1 Exceeded max redirects
-- Msg: max redirects
ripple.app.ValidatorSite Fetch list - /redirect_to/ftp://invalid-url/302 [https] v1 Invalid redirect location
-- Msg: Unsupported scheme: 'ftp'
ripple.app.ValidatorSite Fetch list - /redirect_to/file://invalid-url/302 [https] v1 Invalid redirect location
-- Msg: file URI cannot contain a hostname
ripple.app.ValidatorSite Fetch list - /validators/bad [https] v1 Unable to parse JSON response
-- Msg: bad json
ripple.app.ValidatorSite Fetch list - /validators2/bad [https] v1 Unable to parse JSON response
-- Msg: bad json
ripple.app.ValidatorSite Fetch list - /bad-resource [https] v1 returned bad status
-- Msg: bad result code
ripple.app.ValidatorSite Fetch list - /redirect_nolo/308 [https] v1 returned a redirect with no Location
-- Msg: missing location
ripple.app.ValidatorSite Fetch list - /validators/missing [https] v1 Missing fields in JSON response
-- Msg: missing fields
ripple.app.ValidatorSite Fetch list - /validators2/missing [https] v1 Missing fields in JSON response
-- Msg: missing fields
ripple.app.ValidatorSite Fetch list - /sleep/13 [https] v1 took too long
-- Msg: fetch error
ripple.app.ValidatorSite Fetch list - /validators [https] v2 Missing fields
-- Msg: missing fields
ripple.app.ValidatorSite Fetch list - /validators2 [https] v0 Missing fields
-- Msg: missing fields
ripple.app.ValidatorSite Fetch list - /validators [https] v4 Missing fields
-- Msg: missing fields

Maybe it's not an issue, but I think we could get around this by changing the default log level in the Env constructor.

@scottschurr
Copy link
Collaborator Author

@undertome, yup, I saw those too. Much of the noise from ValidatorSite results from the way that the names of the tests are generated, not from writes to the log. I didn't feel like I had the expertise to monkey with the test naming for that particular test. Yes, there's more noise that could be removed, but I feel like the commit as it stands is a good start.

Copy link
Collaborator

@ximinez ximinez 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, and removes a lot of extra output. For reference, here's what's left after I strip out all the test names. That's about 150 fewer lines of extra output from develop.

7>  -- Msg: max redirects
7>  -- Msg: max redirects
7>  -- Msg: max redirects
7>  -- Msg: max redirects
7>  -- Msg: max redirects
7>  -- Msg: Unsupported scheme: 'ftp'
7>  -- Msg: file URI cannot contain a hostname
7>  -- Msg: bad json
7>  -- Msg: bad json
7>  -- Msg: bad result code
7>  -- Msg: missing location
7>  -- Msg: missing fields
7>  -- Msg: missing fields
7>  -- Msg: fetch error
7>  -- Msg: missing fields
7>  -- Msg: missing fields
7>  -- Msg: missing fields
14> expected_probe_count_min: 10
expected_probe_count_max: 10
7>  -- Msg: max redirects
7>  -- Msg: max redirects
7>  -- Msg: max redirects
7>  -- Msg: max redirects
7>  -- Msg: max redirects
7>  -- Msg: Unsupported scheme: 'ftp'
7>  -- Msg: file URI cannot contain a hostname
7>  -- Msg: bad json
7>  -- Msg: bad json
7>  -- Msg: bad result code
7>  -- Msg: missing location
7>  -- Msg: missing fields
7>  -- Msg: missing fields
7>  -- Msg: fetch error
7>  -- Msg: missing fields
7>  -- Msg: missing fields
7>  -- Msg: missing fields
7>  -- Msg: bad json
7>  -- Msg: fetch error
7>  -- Msg: fetch error
7> Skipping rm dir: test_fetch1
11> 2E81FC6EC0DD943197E0C7E3FBE9AE307F2775F2F7485BB37307984C3C0F23409ED78915C6325F2EC67182FC916040556592DEDF7E8EEF67686570D4B2CC3053437CFC44ED39E3E58650C80C023EB01B7AA4E5E20BBF61A8A59FC2A711A38BEB11> ripple.app.PayStrand To Strand
12> Skipping rm dir: test_db1
12> Skipping rm dir: test_cfg6
12> Skipping rm dir: test_cfg7
12> Skipping rm dir: test_cfg8
Loading: "testSetup12\rippled.cfg"
12> Skipping rm dir: test_cfg9
12> Skipping rm dir: test_cfg9
Loading: "testSetup12\rippled.cfg"
Loading: "testSetup13\rippled.cfg"
Loading: "testSetup13\rippled.cfg"
12> Opened '' (ip=127.0.0.1:0, http)
12> server listening on port 60946
12> #1 accept:    127.0.0.1
12> #1 destroyed: 1 request
12> #2 accept:    127.0.0.1
12> #2 destroyed: 2 requests
10> [server] up on port: 51972
template_mismatch: Field 'Account' is required but missing.
template_mismatch: Field 'SignerWeight' is required but missing.
template_mismatch: Field 'Amount' found in disallowed location.
template_mismatch: Field 'Account' is required but missing.
15> Checking destination invariants...

@scottschurr
Copy link
Collaborator Author

@undertome, I've reduced the unit test noise further by removing some logging that I found I the unit tests.

@scottschurr
Copy link
Collaborator Author

Thanks for the review @undertome.

@ximinez, do you want to take a second look at this pull request since I added a [FOLD] commit? Or are you good? Thanks.

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Still looks good

@scottschurr scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 14, 2022
@nbougalis nbougalis mentioned this pull request Jul 11, 2022
@scottschurr scottschurr deleted the log-noise branch September 1, 2022 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Tech Debt Non-urgent improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants