roachtest: UT for test selection #130079

nameisbhaskar · 2024-09-04T14:09:10Z

This PR has the unit tests for the test selection changes. The tests the selector and the main code. This does not validate the snowflake query, but, simulates the response for other tests.

With this change, we have a coverage of 96% for the test selection.

Fixes: #129786
Epic: None

cockroach-teamcity · 2024-09-04T14:09:21Z

This change is

DarrylWong

I haven't taken a deep look at this PR yet, but wrt #130330, it's good that it caught that bug, but the fact it was just an occasional flake makes me think we should add a bit more. We should have all the parts to do a full simulation (in addition to these hard coded edge cases).

i.e. generate some random starting state, run test selection, randomly pass/fail/preempt some tests, mock writing back the data, then repeat over N days. You can then check invariants at each step, i.e. preempted test is selected the next day, all tests have been run in last 4 days, etc.

Wdyt? Could also be a followup, since it might be a non trivial amount of code.

nameisbhaskar · 2024-09-10T10:08:39Z

I haven't taken a deep look at this PR yet, but wrt #130330, it's good that it caught that bug, but the fact it was just an occasional flake makes me think we should add a bit more. We should have all the parts to do a full simulation (in addition to these hard coded edge cases).

i.e. generate some random starting state, run test selection, randomly pass/fail/preempt some tests, mock writing back the data, then repeat over N days. You can then check invariants at each step, i.e. preempted test is selected the next day, all tests have been run in last 4 days, etc.

Wdyt? Could also be a followup, since it might be a non trivial amount of code.

The current tests covers all known scenarios. But, randomised test is a good idea to generate Fuzzy kind of test cases - I will check how this can be achieved.

nameisbhaskar · 2024-09-10T13:58:04Z

@DarrylWong randomised test is making it complex for the assertion, but it should be doable. But, I am thinking of raising another PR later for that. Please do let me know if that sound ok to you.

pkg/cmd/roachtest/main_test.go

DarrylWong · 2024-09-20T20:02:34Z

pkg/cmd/roachtest/main_test.go

+	testselector.SqlConnectorFunc = func(_, _ string) (*gosql.DB, error) {
+		return db, err
+	}
+	t.Run("expect CategoriseTests to fail", func(t *testing.T) {


From my understanding we aren't testing to see if CategoriseTests will fail, but rather that if it does we should expect the test selector to fall back to all tests selected. Could we make that more clear through the test name + comments?

I will change the test name to "expect CategoriseTests to fail which causes fall back to run all tests". Also, will add comment.

DarrylWong · 2024-09-20T20:24:09Z

pkg/cmd/roachtest/main_test.go

+	for _, s := range specs {
+		s.Suites = registry.Suites(registry.Nightly, registry.Weekly)
+	}
+	data := [][]string{


For those less familiar with the snowflake query, can you leave a comment explaining what the four fields are below?

DarrylWong · 2024-09-20T20:40:02Z

pkg/cmd/roachtest/main_test.go

+// all tests which contains "_randomized" are set in spec as Randomized=true
+func getMockData() ([]registry.TestSpec, *sqlmock.Rows) {
+	specs := []registry.TestSpec{
+		{Name: "t2_selected"},


nit: what's the naming convention of these test names? 😅 Some have a number after t, some have it at the end of the name, some don't have a number, some are out of order. If there's a reason to not make it standardized lets add a comment.

Sure, I will add a comments. The entries have been manually shuffled around and the some names were added later, so, is the mismatch in names.

pkg/cmd/roachtest/main_test.go

DarrylWong · 2024-09-20T21:10:40Z

pkg/cmd/roachtest/main_test.go

+// all tests which contains "new_test" are present in specs, but missing in snowflake rows
+// all tests which contains "_randomized" are set in spec as Randomized=true
+func getMockData() ([]registry.TestSpec, *sqlmock.Rows) {
+	specs := []registry.TestSpec{


Can we leave comments explaining what case we are actually testing for? i.e. for t_new_test_3 we expect that it's still selected as there's no data in snowflake even though it is not part of the 30% selection rate.

nameisbhaskar · 2024-09-22T17:08:35Z

Thanks @DarrylWong for your review. I have addressed the comments. Please do take a look.

DarrylWong

This mostly looks good to me. My remaining comments are mostly just noting that some tests seem redundant to me.

DarrylWong · 2024-09-23T15:59:59Z

pkg/cmd/roachtest/testselector/selector_test.go

+	t.Run("expect Prepare to fail", func(t *testing.T) {
+		db, mock, err = sqlmock.New()
+		require.Nil(t, err)
+		mock.ExpectPrepare(regexp.QuoteMeta(PreparedQuery)).WillReturnError(fmt.Errorf("failed to prepare"))


This seems like it's testing the sql mock more than it is the CategoriseTests. No harm in keeping it but I feel like expect CategoriseTests to fail which causes fall back to run all tests test above already covers this case.

This is to test that we are returning the error correctly from the code.

pkg/cmd/roachtest/testselector/selector_test.go

DarrylWong · 2024-09-23T17:31:29Z

pkg/cmd/roachtest/testselector/selector_test.go

+	require.Equal(t, defaultLastRunOn, req.LastRunOn)
+}
+
+func Test_getSFCreds(t *testing.T) {


Similar comment to above, these tests seem to be already covered by TestCategoriseTests? Not sure how much extra coverage we get.

This is for the coverage of all error scenarios and ensure that we are returning the errors.

pkg/cmd/roachtest/main_test.go

nameisbhaskar · 2024-09-24T06:49:23Z

Thanks @DarrylWong ! I have replied to your comments. Please do let me know if it sounds good.

This PR has the unit tests for the test selection changes. The tests the selector and the main code. This does not validate the snowflake query, but, simulates the response for other tests. Fixes: cockroachdb#129786 Epic: None

nameisbhaskar · 2024-09-24T14:29:43Z

Thanks @DarrylWong for reviewing this.

nameisbhaskar · 2024-09-24T14:29:52Z

bors r=@DarrylWong

craig · 2024-09-24T14:57:04Z

Build succeeded:

nameisbhaskar force-pushed the bhaskar-ut-test-selection branch 4 times, most recently from 2c9a239 to e26b555 Compare September 5, 2024 15:53

nameisbhaskar marked this pull request as ready for review September 6, 2024 05:56

nameisbhaskar requested a review from a team as a code owner September 6, 2024 05:56

nameisbhaskar requested review from herkolategan and DarrylWong and removed request for a team September 6, 2024 05:56

nameisbhaskar force-pushed the bhaskar-ut-test-selection branch 2 times, most recently from b3e692e to 1c05103 Compare September 9, 2024 05:59

nameisbhaskar marked this pull request as draft September 9, 2024 13:23

nameisbhaskar removed request for herkolategan and DarrylWong September 9, 2024 13:23

DarrylWong reviewed Sep 9, 2024

View reviewed changes

nameisbhaskar force-pushed the bhaskar-ut-test-selection branch from 1c05103 to 83d0236 Compare September 10, 2024 09:21

nameisbhaskar requested a review from DarrylWong September 10, 2024 13:58

nameisbhaskar marked this pull request as ready for review September 10, 2024 16:48

srosenberg requested a review from herkolategan September 17, 2024 16:58

nameisbhaskar force-pushed the bhaskar-ut-test-selection branch from 83d0236 to 93a0ee3 Compare September 17, 2024 17:04

nameisbhaskar requested a review from srosenberg September 17, 2024 17:07

DarrylWong reviewed Sep 20, 2024

View reviewed changes

nameisbhaskar force-pushed the bhaskar-ut-test-selection branch from 93a0ee3 to a6be180 Compare September 22, 2024 17:07

cockroachdb deleted a comment from bitomukesh Sep 22, 2024

nameisbhaskar requested a review from DarrylWong September 22, 2024 17:21

DarrylWong reviewed Sep 23, 2024

View reviewed changes

nameisbhaskar force-pushed the bhaskar-ut-test-selection branch from a6be180 to 786f5eb Compare September 24, 2024 06:48

nameisbhaskar force-pushed the bhaskar-ut-test-selection branch from 786f5eb to 147ac90 Compare September 24, 2024 06:51

nameisbhaskar requested a review from DarrylWong September 24, 2024 06:52

roachtest: UT for test selection

fce0b2d

This PR has the unit tests for the test selection changes. The tests the selector and the main code. This does not validate the snowflake query, but, simulates the response for other tests. Fixes: cockroachdb#129786 Epic: None

nameisbhaskar force-pushed the bhaskar-ut-test-selection branch from 147ac90 to fce0b2d Compare September 24, 2024 07:02

DarrylWong approved these changes Sep 24, 2024

View reviewed changes

craig bot merged commit a2bbbd6 into cockroachdb:master Sep 24, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: UT for test selection #130079

roachtest: UT for test selection #130079

nameisbhaskar commented Sep 4, 2024 •

edited

Loading

cockroach-teamcity commented Sep 4, 2024

DarrylWong left a comment •

edited

Loading

nameisbhaskar commented Sep 10, 2024

nameisbhaskar commented Sep 10, 2024 •

edited

Loading

DarrylWong Sep 20, 2024

nameisbhaskar Sep 22, 2024

DarrylWong Sep 20, 2024

DarrylWong Sep 20, 2024

nameisbhaskar Sep 22, 2024

DarrylWong Sep 20, 2024

nameisbhaskar commented Sep 22, 2024

DarrylWong left a comment

DarrylWong Sep 23, 2024

nameisbhaskar Sep 24, 2024

DarrylWong Sep 23, 2024

nameisbhaskar Sep 24, 2024

nameisbhaskar commented Sep 24, 2024

nameisbhaskar commented Sep 24, 2024

nameisbhaskar commented Sep 24, 2024

craig bot commented Sep 24, 2024

roachtest: UT for test selection #130079

roachtest: UT for test selection #130079

Conversation

nameisbhaskar commented Sep 4, 2024 • edited Loading

cockroach-teamcity commented Sep 4, 2024

DarrylWong left a comment • edited Loading

Choose a reason for hiding this comment

nameisbhaskar commented Sep 10, 2024

nameisbhaskar commented Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nameisbhaskar commented Sep 22, 2024

DarrylWong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nameisbhaskar commented Sep 24, 2024

nameisbhaskar commented Sep 24, 2024

nameisbhaskar commented Sep 24, 2024

craig bot commented Sep 24, 2024

nameisbhaskar commented Sep 4, 2024 •

edited

Loading

DarrylWong left a comment •

edited

Loading

nameisbhaskar commented Sep 10, 2024 •

edited

Loading