Skip to content

Conversation

Sachuman
Copy link
Contributor

@Sachuman Sachuman commented Sep 2, 2025

Added tests for EstimateDiskUsage and EstimateDiskUsageByBackingType.
Added calls to metamorphic tests, not for results but for races/panics.

Implements #5235

@Sachuman Sachuman requested a review from a team as a code owner September 2, 2025 19:03
@Sachuman Sachuman requested a review from xxmplus September 2, 2025 19:03
@Sachuman Sachuman marked this pull request as draft September 2, 2025 19:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@Sachuman Sachuman force-pushed the adding-tests-for-EstimateDiskUsage branch 2 times, most recently from 6f5050d to e380391 Compare September 3, 2025 16:27
@Sachuman Sachuman marked this pull request as ready for review September 3, 2025 17:25
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

@jbowens reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @xxmplus)


disk_usage_test.go line 155 at r1 (raw file):

				case "zero":
					require.Equal(t, uint64(0), total)
				case "non-zero":

rather than just expecting zero or nonzero, can we include an explicit value in the test results? It'll help catch instances where the calculated size changes unexpected/unintentionally.


metamorphic/generator.go line 418 at r1 (raw file):

}

func (g *generator) dbEstimateDiskUsage() {

nice

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm: Very nice!

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @jbowens, @Sachuman, and @xxmplus)


disk_usage_test.go line 155 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

rather than just expecting zero or nonzero, can we include an explicit value in the test results? It'll help catch instances where the calculated size changes unexpected/unintentionally.

+1 We'll probably need to set DisableAutomaticCompactions in the options.

@Sachuman Sachuman force-pushed the adding-tests-for-EstimateDiskUsage branch 3 times, most recently from bf8e0f6 to 082611f Compare September 4, 2025 15:24
Copy link
Contributor Author

@Sachuman Sachuman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @jbowens and @xxmplus)


disk_usage_test.go line 155 at r1 (raw file):

Previously, RaduBerinde wrote…

+1 We'll probably need to set DisableAutomaticCompactions in the options.

Seems like even after DisableAutomaticCompactions set, for ingest-external, there is non-determinism for the size,
So I added the expect parameter as optional.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @jbowens, @Sachuman, and @xxmplus)


disk_usage_test.go line 155 at r1 (raw file):

Previously, Sachuman (Sachin) wrote…

Seems like even after DisableAutomaticCompactions set, for ingest-external, there is non-determinism for the size,
So I added the expect parameter as optional.

There are other tests that use ingestions and are deterministic. I think we need to specify the block sizes to avoid the randomization in runBuildRemoteCmd

If it still doesn't work, instead of expect-total you can add a flag that changes any non-zero value to a special <non-zero> string when outputting (so we can continue to rely on matching test output)

Added tests for EstimateDiskUsage and EstimateDiskUsageByBackingType.
Added calls to metamorphic tests, not for results but for races/panics
@Sachuman Sachuman force-pushed the adding-tests-for-EstimateDiskUsage branch from 082611f to 5eb27bf Compare September 4, 2025 16:09
Copy link
Contributor Author

@Sachuman Sachuman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @jbowens and @xxmplus)


disk_usage_test.go line 155 at r1 (raw file):

Previously, RaduBerinde wrote…

There are other tests that use ingestions and are deterministic. I think we need to specify the block sizes to avoid the randomization in runBuildRemoteCmd

If it still doesn't work, instead of expect-total you can add a flag that changes any non-zero value to a special <non-zero> string when outputting (so we can continue to rely on matching test output)

Specifying the block size worked, I did not realize it was random there. Thanks

Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @Sachuman and @xxmplus)

@Sachuman
Copy link
Contributor Author

Sachuman commented Sep 4, 2025

TFTRs!

@Sachuman Sachuman merged commit 67288c5 into cockroachdb:master Sep 4, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants