Add rivertype.JobStates with full list of job states
#297
Conversation
brandur
force-pushed
the
brandur-job-states
branch
2 times, most recently
from
efcecae
to
11bdd7b
Compare
brandur
force-pushed
the
brandur-job-states
branch
2 times, most recently
from
493f653
to
7b8e838
Compare
| // they're included in JobStates. Helps check that we didn't add a new value | ||
| // but forgot to add it to the full list of constant values. | ||
| for _, nameAndValue := range allValuesForStringConstantType(t, "river_type.go", "JobState") { | ||
| t.Logf("Checking for job state: %s / %s", nameAndValue.Name, nameAndValue.Value) |
There was a problem hiding this comment.
=== RUN TestJobStates
=== PAUSE TestJobStates
=== CONT TestJobStates
river_type_test.go:27: Checking for job state: JobStateAvailable / available
river_type_test.go:27: Checking for job state: JobStateCancelled / cancelled
river_type_test.go:27: Checking for job state: JobStateCompleted / completed
river_type_test.go:27: Checking for job state: JobStateDiscarded / discarded
river_type_test.go:27: Checking for job state: JobStateRetryable / retryable
river_type_test.go:27: Checking for job state: JobStateRunning / running
river_type_test.go:27: Checking for job state: JobStateScheduled / scheduled
--- PASS: TestJobStates (0.00s)
|
Note that #301 adds a new |
| // Get all job state names from the corresponding source file and make sure | ||
| // they're included in JobStates. Helps check that we didn't add a new value | ||
| // but forgot to add it to the full list of constant values. | ||
| for _, nameAndValue := range allValuesForStringConstantType(t, "river_type.go", "JobState") { |
rivertype/river_type_test.go
Outdated
| } | ||
|
|
||
| if len(valueNames) < 1 { | ||
| require.FailNow(t, "Not values found", "No values found for source file and constant type: %s / %s", srcFile, typeName) |
There was a problem hiding this comment.
typo?
| require.FailNow(t, "Not values found", "No values found for source file and constant type: %s / %s", srcFile, typeName) | |
| require.FailNow(t, "No values found", "No values found for source file and constant type: %s / %s", srcFile, typeName) |
There was a problem hiding this comment.
Yep, or probably Vim-o as I was trying to do something and accidentally added/replaced a character. Thanks!
Ah good to know. Welp, tests should fail, so hopefully we'll be forced to get this right. |
brandur
force-pushed
the
brandur-job-states
branch
from
7b8e838
to
e2b098b
Compare
Requested in #296, add a new `JobStates()` function that contains a complete list of all `JobState*` constant values, which may be useful in places like testing. Also add a test that does a basic parsing of the corresponding Go file, look for all `JobState*` values, and makes sure that `JobStates()` contains every possible value to prevent future regressions. Fixes #296.
brandur
force-pushed
the
brandur-job-states
branch
from
e2b098b
to
59acac6
Compare
|
Thx. |
|
Thanks for doing this @brandur! I was thinking you'd end up implementing this as something like a check against the output of SELECT
n.nspname AS schema,
e.enumlabel AS enum_value
FROM
pg_catalog.pg_type AS t
LEFT JOIN pg_catalog.pg_namespace AS n ON n.oid = t.typnamespace
LEFT JOIN pg_catalog.pg_enum AS e ON e.enumtypid = t.oid
WHERE
t.typname = 'river_job_state'
ORDER BY e.enumsortorder;In our setup, where we put you guys in I'm happy to implement this as a unit test if you'd like! |
|
@dhermes this would be fine for a unit test, though I’m not sure it’s worth the effort. I wouldn’t want to base the actual API on that query though (not sure if this is what you were suggesting). While I don’t expect to add more states beyond this, #301 does add a new |
Haha sorry I was not trying to suggest that. Definitely not.
I've got some existing unit tests like this in our codebase that leverage For me, it's just about trying to get the best of both worlds of Go and PostgreSQL. I.e. using the right native SQL / PostgreSQL capabilities for enums, and the right Go capabilities for enums. And then just making sure we have a guarantee at dev time that the native Go and native PostgreSQL are agreeing with each other. If we're not aligned on this, all good. I'm not trying to step on any toes, it's your project! (And we love this project, it's awesome!) |
A small change that removes a list of all job states defined in `insert_opts.go` that was used to validate states sent into `UniqueOpts` with `rivertype.JobStates()` which was introduced recently n #297. Removing one of the lists means we only need to maintain one of them. I left the variable in place instead of invoking `rivertype.JobStates()` so that we don't need to initialize a new slice every time we validate.
A small change that removes a list of all job states defined in `insert_opts.go` that was used to validate states sent into `UniqueOpts` with `rivertype.JobStates()` which was introduced recently n #297. Removing one of the lists means we only need to maintain one of them. I left the variable in place instead of invoking `rivertype.JobStates()` so that we don't need to initialize a new slice every time we validate.
A small change that removes a list of all job states defined in `insert_opts.go` that was used to validate states sent into `UniqueOpts` with `rivertype.JobStates()` which was introduced recently n #297. Removing one of the lists means we only need to maintain one of them. I left the variable in place instead of invoking `rivertype.JobStates()` so that we don't need to initialize a new slice every time we validate.
A small change that removes a list of all job states defined in `insert_opts.go` that was used to validate states sent into `UniqueOpts` with `rivertype.JobStates()` which was introduced recently n #297. Removing one of the lists means we only need to maintain one of them. I left the variable in place instead of invoking `rivertype.JobStates()` so that we don't need to initialize a new slice every time we validate.
Requested in #296, add a new
JobStates()function that contains acomplete list of all
JobState*constant values, which may be useful inplaces like testing.
Also add a test that does a basic parsing of the corresponding Go file,
look for all
JobState*values, and makes sure thatJobStates()contains every possible value to prevent future regressions.
Fixes #296.