chore: cover postgres_jobs.go #8841

kkunapuli · 2024-02-13T20:48:45Z

Description

Add integration test coverage for postgres_jobs.go - DET-10133.

Test Plan

Run integration tests locally, and via CI. Ensure they pass.

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

DET-10133

netlify · 2024-02-13T20:48:50Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`440cbf7`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/65cf793cf4320b0008560a14

codecov · 2024-02-13T20:48:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a4dbc03) 47.70% compared to head (440cbf7) 47.71%.
Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8841   +/-   ##
=======================================
  Coverage   47.70%   47.71%           
=======================================
  Files        1065     1065           
  Lines      169614   169614           
  Branches     2238     2238           
=======================================
+ Hits        80920    80930   +10     
+ Misses      88536    88526   -10     
  Partials      158      158

Flag	Coverage Δ
backend	`43.55% <ø> (+0.02%)`	⬆️
harness	`64.11% <ø> (ø)`
web	`42.55% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

carolinaecalderon

Great work, a couple notes/links on writing table-driven tests & the golang 'testing' package.

carolinaecalderon · 2024-02-14T22:28:18Z

master/internal/db/postgres_jobs_intg_test.go

+func setupDBForTest(t *testing.T) *PgDB {
+	require.NoError(t, etc.SetRootPath(RootFromDB))
+
+	db := MustResolveTestPostgres(t)
+	MustMigrateTestPostgres(t, db, MigrationsFromDB)
+	return db
+}


nit: replace this with TestMain -- see other TestMains in the repo for more context. Then, you can reference it in the other test functions with db.SingleDB()
https://medium.com/goingogo/why-use-testmain-for-testing-in-go-dafb52b406bc

TestMain(...) is so clean. Unfortunately, the db package has a lot of integration test files, some of which are closing db connections in an attempt to ensure they are using a fresh/clean database. Details here: https://hpe-aiatscale.atlassian.net/browse/RM-27

This means that until they are refactored to their own package, tests need to establish their own database connection instead of using TestMain(...).

Good to know, I think all the tests I've written have been outside the 'db' package so I hadn't run into this before.

It was a good experience to learn about TestMain and implement the change even if it didn't work out! Now I know to be aware of it in the future. :)

carolinaecalderon · 2024-02-14T22:30:51Z

master/internal/db/postgres_jobs_intg_test.go

+		assert.Equal(t, sendJob.JobID, recvJob.JobID)
+		assert.Equal(t, sendJob.JobType, recvJob.JobType)
+		assert.Equal(t, sendJob.OwnerID, recvJob.OwnerID)
+		assert.Equal(t, decimal.NewFromInt(10).Equal(recvJob.QPos), true)


is there a reason you can't assert.Equal(t, decimal.NewFromInt(10), recvJob.QPos)?

Yes, two decimal.Decimal variables are not equal even if their values are equal. We need to use the .Equal() method to test for the values being equal.

I made a playground to demonstrate: https://go.dev/play/p/asTPVa4OuQZ

carolinaecalderon · 2024-02-14T22:32:49Z

master/internal/db/postgres_jobs_intg_test.go

+func TestUpdateJobPosition(t *testing.T) {
+	db := setupDBForTest(t)
+
+	t.Run("update position", func(t *testing.T) {
+		// create and send job
+		origPos := decimal.NewFromInt(10)
+		newPos := decimal.NewFromInt(5)
+		sendJob := model.Job{
+			JobID:   model.NewJobID(),
+			JobType: model.JobTypeExperiment,
+			QPos:    origPos,
+		}
+		err := db.AddJob(&sendJob)
+		require.NoError(t, err)
+
+		// update job position
+		err = db.UpdateJobPosition(sendJob.JobID, newPos)
+		require.NoError(t, err)
+
+		// retrieve job and confirm pos update
+		recvJob, err := db.JobByID(sendJob.JobID)
+		require.NoError(t, err)
+		assert.Equal(t, newPos.Equal(recvJob.QPos), true)
+	})
+
+	t.Run("update position - negative value", func(t *testing.T) {
+		// create and send job
+		origPos := decimal.NewFromInt(10)
+		newPos := decimal.NewFromInt(-5)
+		sendJob := model.Job{
+			JobID:   model.NewJobID(),
+			JobType: model.JobTypeExperiment,
+			QPos:    origPos,
+		}
+		err := db.AddJob(&sendJob)
+		require.NoError(t, err)
+
+		// update job position
+		err = db.UpdateJobPosition(sendJob.JobID, newPos)
+		require.NoError(t, err)
+
+		// retrieve job and confirm pos update
+		recvJob, err := db.JobByID(sendJob.JobID)
+		require.NoError(t, err)
+		assert.Equal(t, newPos.Equal(recvJob.QPos), true)
+	})
+
+	t.Run("update position - empty ID", func(t *testing.T) {
+		// create and send job
+		origPos := decimal.NewFromInt(10)
+		newPos := decimal.NewFromInt(5)
+		sendJob := model.Job{
+			JobID:   model.NewJobID(),
+			JobType: model.JobTypeExperiment,
+			QPos:    origPos,
+		}
+		err := db.AddJob(&sendJob)
+		require.NoError(t, err)
+
+		// update job position
+		err = db.UpdateJobPosition(model.JobID(""), newPos)
+		require.Error(t, err)
+
+		// retrieve job and ensure queue pos not updated
+		recvJob, err := db.JobByID(sendJob.JobID)
+		require.NoError(t, err)
+		assert.Equal(t, origPos.Equal(recvJob.QPos), true)
+	})
+
+	t.Run("update position - ID does not exist", func(t *testing.T) {
+		// create and send job
+		origPos := decimal.NewFromInt(10)
+		newPos := decimal.NewFromInt(5)
+		sendJob := model.Job{
+			JobID:   model.NewJobID(),
+			JobType: model.JobTypeExperiment,
+			QPos:    origPos,
+		}
+		err := db.AddJob(&sendJob)
+		require.NoError(t, err)
+
+		// update job position for a job that doesn't exist
+		err = db.UpdateJobPosition(model.NewJobID(), newPos)
+		require.NoError(t, err)
+	})
+}


blocker: since this test has a lot of repeated code, I recommend setting this test up with test cases and then looping through them, see https://dave.cheney.net/2019/05/07/prefer-table-driven-tests or search for cases := []struct { in the repo.

I appreciate the desire to limit repeated code! I also see your point - but I don't think table tests are the best approach here.

the tests aren't exact copies of each other; I think it will add complexity to try and fit them all in the same table test. Basically, there's two types of variations: position value and job ID.

you can't run a single test case with table tests (as far as I can tell); since we prefer require over assert, this means the test stops at the first test case that fails. With the existing code structure we can run individual cases or run TestUpdateJobPosition and get info on all failing cases

Repeated code is usually more tolerated in test code because the purpose is a little different. It's more important to be usable and to easily understand what a test is doing.

I think a nice alternative might be to extract some of the repeated code into a helper function. How does that sound to you?

I think that's a good idea

carolinaecalderon

LGTM, great work

* chore: cover postgres_go * gofumpt update * remove comments; entered by accident * update from make fmt * fix check issues * use TestMain in db package to setup db for tests * reduce amount of duplicate code with utility function * additional cleanup * initialize db outside of TestMain until enough isolation is achieved

chore: cover postgres_go

caea9db

cla-bot bot added the cla-signed label Feb 13, 2024

kkunapuli added 4 commits February 13, 2024 15:49

gofumpt update

f5ea0aa

remove comments; entered by accident

90d8568

update from make fmt

a0ab365

fix check issues

6c25458

kkunapuli marked this pull request as ready for review February 13, 2024 21:59

kkunapuli requested a review from a team as a code owner February 13, 2024 21:59

kkunapuli requested a review from ioga February 13, 2024 21:59

kkunapuli changed the title ~~chore: cover postgres_go~~ chore: cover postgres_jobs.go Feb 13, 2024

kkunapuli requested a review from carolinaecalderon February 14, 2024 21:58

carolinaecalderon requested changes Feb 14, 2024

View reviewed changes

kkunapuli added 4 commits February 15, 2024 13:18

use TestMain in db package to setup db for tests

69e9d47

reduce amount of duplicate code with utility function

bdb30bc

additional cleanup

fb12b13

initialize db outside of TestMain until enough isolation is achieved

440cbf7

kkunapuli requested a review from carolinaecalderon February 16, 2024 15:27

carolinaecalderon approved these changes Feb 16, 2024

View reviewed changes

kkunapuli merged commit de28a57 into main Feb 16, 2024
74 of 86 checks passed

kkunapuli deleted the kunapuli/cover-postgres_jobs branch February 16, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: cover postgres_jobs.go #8841

chore: cover postgres_jobs.go #8841

kkunapuli commented Feb 13, 2024 •

edited by jira bot

Loading

netlify bot commented Feb 13, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

carolinaecalderon left a comment

carolinaecalderon Feb 14, 2024

kkunapuli Feb 15, 2024

kkunapuli Feb 15, 2024

carolinaecalderon Feb 16, 2024

kkunapuli Feb 16, 2024

carolinaecalderon Feb 14, 2024

kkunapuli Feb 14, 2024 •

edited

Loading

carolinaecalderon Feb 14, 2024

kkunapuli Feb 14, 2024 •

edited

Loading

carolinaecalderon Feb 16, 2024

carolinaecalderon left a comment

chore: cover postgres_jobs.go #8841

chore: cover postgres_jobs.go #8841

Conversation

kkunapuli commented Feb 13, 2024 • edited by jira bot Loading

Description

Test Plan

Checklist

Ticket

netlify bot commented Feb 13, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented Feb 13, 2024 • edited Loading

Codecov Report

carolinaecalderon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkunapuli Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkunapuli Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolinaecalderon left a comment

Choose a reason for hiding this comment

kkunapuli commented Feb 13, 2024 •

edited by jira bot

Loading

netlify bot commented Feb 13, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

kkunapuli Feb 14, 2024 •

edited

Loading

kkunapuli Feb 14, 2024 •

edited

Loading