chore: cover and bunify project functions in postgres_experiments.go #8912

kkunapuli · 2024-02-28T00:47:50Z

Description

RM-48 chore: cover and bunify project functions in postgres_experiments.go

Test Plan

ensure all unit/integration tests pass; there should be no change in behavior

ProjectExperiments is called when deleting a workspace. Create a workspace, add a project, and an experiment. Then, delete it. It completes without error.

Repeat above with deleting a project from a workspace.

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

RM-48

netlify · 2024-02-28T00:48:06Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`2d7e83a`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/65dfc2c59a779d000835c605

codecov · 2024-02-28T00:52:08Z

Codecov Report

Attention: Patch coverage is 77.61194% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 47.54%. Comparing base (4618389) to head (2d7e83a).
Report is 16 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8912   +/-   ##
=======================================
  Coverage   47.53%   47.54%           
=======================================
  Files        1066     1068    +2     
  Lines      170434   170513   +79     
  Branches     2235     2237    +2     
=======================================
+ Hits        81015    81066   +51     
- Misses      89261    89289   +28     
  Partials      158      158

Flag	Coverage Δ
backend	`43.39% <77.61%> (+0.04%)`	⬆️
harness	`63.69% <ø> (ø)`
web	`42.53% <ø> (ø)`

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

Files	Coverage Δ
master/internal/api_experiment.go	`54.84% <ø> (ø)`
master/internal/core_experiment.go	`61.16% <100.00%> (ø)`
master/internal/db/postgres_test_utils.go	`83.21% <100.00%> (+0.24%)`	⬆️
master/internal/api_project.go	`24.59% <0.00%> (ø)`
master/internal/api_workspace.go	`62.78% <0.00%> (ø)`
master/internal/webhooks/postgres_webhook.go	`57.10% <0.00%> (+0.26%)`	⬆️
master/internal/db/postgres_experiments.go	`57.79% <86.95%> (+3.48%)`	⬆️
master/internal/project/postgres_project.go	`66.66% <66.66%> (ø)`

... and 4 files with indirect coverage changes

kkunapuli · 2024-02-28T20:28:04Z

master/internal/workspace/postgres_workspace.go

-	}
-	return &pID, nil
-}
-


moved function to postgres_project.go

kkunapuli · 2024-02-28T20:28:27Z

master/internal/db/postgres_experiments.go

-		return 1, fmt.Errorf("given workspace or project is archived and cannot add new experiments")
-	}
-	return int(p.Id), nil
-}


moved function to postgres_projects.go

kkunapuli · 2024-02-28T21:47:15Z

master/internal/api_experiment_intg_test.go


 	// user is logged in again during experiment creation, meaning args don't match
 	mockUserArg := mock.MatchedBy(func(u model.User) bool {
 		return u.ID == curUser.ID
 	})

-	// Can't view forked experiment.


separated tests so they can be run individually

kkunapuli · 2024-02-28T23:17:13Z

master/internal/api_workspace.go

@@ -527,7 +527,7 @@ func (a *apiServer) deleteWorkspace(
 	log.Debugf("deleting workspace %d projects", workspaceID)
 	holder := &workspacev1.Workspace{}
 	for _, pj := range projects {
-		expList, err := a.m.db.ProjectExperiments(int(pj.Id))
+		expList, err := db.ProjectExperiments(context.TODO(), int(pj.Id))


using context.TODO() here because otherwise I was running into context cancelled errors; I think that defer rows.Close() within ProjectExperiments(...) might be cancelling the context

carolinaecalderon

LGTM; see nit/suggestion

carolinaecalderon · 2024-02-29T16:13:18Z

master/internal/workspace/postgres_workspace.go

-	}
-	return &pID, nil
-}
-


carolinaecalderon · 2024-02-29T16:31:55Z

master/internal/db/postgres_experiments.go

+func ProjectExperiments(ctx context.Context, pID int) (experiments []*model.Experiment, err error) {
+	rows, err := Bun().NewSelect().
+		Model((*model.Experiment)(nil)).
+		Column("experiment.id").
+		Column("state").
+		Column("config").
+		Column("start_time").
+		Column("end_time").
+		Column("archived").
+		Column("owner_id").
+		Column("notes").
+		Column("job_id").
+		Column("project_id").
+		Column("unmanaged").
+		ColumnExpr("u.username as username").
+		Join("JOIN users AS u ON (experiment.owner_id = u.id)").
+		Where("experiment.project_id = ?", pID).
+		Rows(ctx)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("selecting project experiments: %w", err)
 	}
-	defer rows.Close()

+	defer rows.Close()
 	for rows.Next() {
 		var exp model.Experiment
-		if err = rows.StructScan(&exp); err != nil {
-			return nil, errors.Wrap(err, "unable to read experiment from db")
+		if err := Bun().ScanRow(ctx, rows, &exp); err != nil {
+			return nil, fmt.Errorf("reading experiment from db: %w", err)
 		}
 		experiments = append(experiments, &exp)
 	}
+
+	if err := rows.Err(); err != nil {
+		return nil, fmt.Errorf("selecting project experiments: %w", err)
+	}


nit: could you follow the pattern of user.GetUserSetting here so you can avoid scanning rows & running defer rows.Close() to cancel the context?
something like:

exps := []*model.Experiment err := db.Bun().NewSelect().Model(&exp)...Scan(ctx) return exps, err

if you've already tried this, disregard -- but hopefully this allows you to pass in the ctx (and not context.TODO()) to these calls.

Thank you for the suggestion! I'm not going to do this for performance reasons; but it's good to know about the tradeoffs / alternatives.

…8912) * initial commit to save progress * missed a file * remove duplicate func from workspace * fix formatting * fix bun query; do not redefine table nickname * fix git check issues * add clarity to error handling when workspace/project does not exist * remove unnecessary print statements * use new context for ProjectExperiments call * use fresh context for deleting projects, too

initial commit to save progress

9842217

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

missed a file

3395ed0

kkunapuli added 4 commits February 28, 2024 11:40

remove duplicate func from workspace

5164bbe

fix formatting

78c164b

fix bun query; do not redefine table nickname

c5e5cb2

fix git check issues

28aec34

kkunapuli changed the title ~~initial commit to save progress~~ chore: cover and bunify project functions in postgres_experiments.go Feb 28, 2024

kkunapuli commented Feb 28, 2024

View reviewed changes

add clarity to error handling when workspace/project does not exist

e750c10

kkunapuli commented Feb 28, 2024

View reviewed changes

remove unnecessary print statements

831a455

kkunapuli marked this pull request as ready for review February 28, 2024 22:03

kkunapuli requested a review from a team as a code owner February 28, 2024 22:03

kkunapuli requested review from NicholasBlaskey and carolinaecalderon February 28, 2024 22:03

use new context for ProjectExperiments call

38856b4

kkunapuli commented Feb 28, 2024

View reviewed changes

use fresh context for deleting projects, too

2d7e83a

carolinaecalderon approved these changes Feb 29, 2024

View reviewed changes

kkunapuli merged commit dffda27 into main Feb 29, 2024
71 of 83 checks passed

kkunapuli deleted the kunapuli/bunify-postgres-project-experiment branch February 29, 2024 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: cover and bunify project functions in postgres_experiments.go #8912

chore: cover and bunify project functions in postgres_experiments.go #8912

kkunapuli commented Feb 28, 2024 •

edited

Loading

netlify bot commented Feb 28, 2024 •

edited

Loading

codecov bot commented Feb 28, 2024 •

edited

Loading

kkunapuli Feb 28, 2024

carolinaecalderon Feb 29, 2024

kkunapuli Feb 28, 2024

kkunapuli Feb 28, 2024

kkunapuli Feb 28, 2024

carolinaecalderon left a comment

carolinaecalderon Feb 29, 2024

carolinaecalderon Feb 29, 2024

kkunapuli Feb 29, 2024

chore: cover and bunify project functions in postgres_experiments.go #8912

chore: cover and bunify project functions in postgres_experiments.go #8912

Conversation

kkunapuli commented Feb 28, 2024 • edited Loading

Description

Test Plan

Checklist

Ticket

netlify bot commented Feb 28, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented Feb 28, 2024 • edited Loading

Codecov Report

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

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

kkunapuli commented Feb 28, 2024 •

edited

Loading

netlify bot commented Feb 28, 2024 •

edited

Loading

codecov bot commented Feb 28, 2024 •

edited

Loading