sql: adjust a couple of memory monitoring tests #100550
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/builtin_mem_usage_test.go line 29 at r1 (raw file):
// record their memory usage as they build up their result. func TestAggregatesMonitorMemory(t *testing.T) { defer leaktest.AfterTest(t)()
[nit] why not just skip with issue since this isn't testing anything right now?
yuzefovich
force-pushed
the
remove-agg-tests
branch
from
5a8e6db
to
bd648cd
Compare
This commit adjusts a couple of memory monitoring related tests. `TestAggregatesMonitorMemory` has been rewritten to observe the correct memory monitor via `crdb_internal.node_memory_monitors` virtual table. `TestEvaluatedMemoryIsChecked` is just straight up removed. Initially, this test was expected to verify that builtin functions like `repeat` perform memory accounting of the intermediate result via our memory accounting system. However, that changed long time ago in 2b00f15 and now such builtins estimate their result size and return `errStringTooLarge` error, so the test was no longer verifying what it intended. This commit removes this test since we do verify the behavior introduced in 2b00f15 elsewhere (in the logic tests). Release note: None
yuzefovich
force-pushed
the
remove-agg-tests
branch
from
bd648cd
to
7d04684
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/builtin_mem_usage_test.go line 29 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit] why not just skip with issue since this isn't testing anything right now?
I refactored the test to use crdb_internal.node_memory_monitors virtual table. PTAL.
There was a problem hiding this comment.
Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status:
|
TFTR! bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 7d04684 to blathers/backport-release-23.1-100550: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This commit adjusts a couple of memory monitoring related tests.
TestAggregatesMonitorMemoryhas been rewritten to observe the correctmemory monitor via
crdb_internal.node_memory_monitorsvirtual table.TestEvaluatedMemoryIsCheckedis just straight up removed. Initially,this test was expected to verify that builtin functions like
repeatperform memory accounting of the intermediate result via our memory
accounting system. However, that changed long time ago in 2b00f15
and now such builtins estimate their result size and return
errStringTooLargeerror, so the test was no longer verifying what itintended. This commit removes this test since we do verify the behavior
introduced in 2b00f15 elsewhere (in the
logic tests).
Fixes: #79014.
Fixes: #100119.
Release note: None