sql/pgwire: tidy up tests for Exec with placeholders

[tamird]

[petermattis]

What is this about?

[petermattis]

LGTM

[tamird]

This is related to #3819.

[RaduBerinde]

sql/pgwire_test.go, line 363 [r2] (raw file):
Wow.. This is kind of insane. Can't we do something more readable, e.g.

for i := 0; i < 2; i++) {
   if (i == 0) {
      res, err := db.Exec()
   } else {
      res, err := stmt.Exec()
   }
   .. check error ..

Actually, it seems that the errors are supposed to be the same (they both must match test.error right?). I would just run both statements and first compare the errors to check they are the same. And then do the error checking once.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

sql/pgwire_test.go, line 363 [r2] (raw file):
I think this pattern is quite idiomatic in Go. checking if both errors are identical first can lead to some hard to debug error messages; this version produces a more self contained error in case this test fails. One problem is that it'll be hard to tell which variant failed (stmt.Exec or db.Exec) - I'll see about improving that.

---

Comments from the review on Reviewable.io

[bdarnell]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

sql/pgwire_test.go, line 363 [r2] (raw file):
I'm not sure what you're referring to by "this pattern", but I wouldn't consider defining functions inline in a for statement to be idiomatic. I prefer Radu's version looping from 0..2. (the error checking is fine as-is)

---

Comments from the review on Reviewable.io

[RaduBerinde]

sql/pgwire_test.go, line 363 [r2] (raw file):
You can also separate the three ifs into a function (which might be useful for other cases as well) and just call it twice (no loop).

res, err := db.Exec()
checkErrorMatches(err, test.error);
res, err = stmt.Exec()
checkErrorMatches(err, test.error)

---

Comments from the review on Reviewable.io

[nvanbenschoten]

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.

---

sql/pgwire_test.go, line 346 [r1] (raw file):
You might want to s/queryTests/execTests/g, and s/query/exec/g below.

---

sql/pgwire_test.go, line 383 [r1] (raw file):
You're not defering anything, so why is this needed?

---

sql/pgwire_test.go, line 363 [r2] (raw file):
Personally I'd be ok with this approach if the function slice wasn't defined inline, but instead assigned to a well-named variable. It seems like we're doing the same thing in TestPGPreparedQuery, so whatever we decide, I'd make sure to keep them consistent.

---

Comments from the review on Reviewable.io

[petermattis]

LGTM modulo some of the other comments below.

---

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.

---

sql/pgwire_test.go, line 262 [r3] (raw file):
Not sure the extra indentation is worth being able to defer rows.Close(), but whatever.

---

Comments from the review on Reviewable.io

[tamird]

PTAL, this got a bit bigger.

---

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.

---

sql/pgwire_test.go, line 346 [r1] (raw file):
changed queryTests, but query is still appropriate since it's a string.

---

sql/pgwire_test.go, line 383 [r1] (raw file):
Removed!

---

sql/pgwire_test.go, line 363 [r2] (raw file):
I don't really see the big problem here; this inline function is just currying db.Exec

---

Comments from the review on Reviewable.io

[maddyblue]

LGTM

---

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from the review on Reviewable.io