atlasaction: migrate apply command #64
Conversation
Atlas Lint ReportAnalyzed migrations2
Migrations automatically reviewed by Atlas |
||||||||||||||||
Atlas Lint ReportAnalyzed migrations
Migrations automatically reviewed by Atlas |
||||||||||||||||
atlasaction/action_test.go
Outdated
| tt.setInput("tx-mode", "fake") | ||
| err := MigrateApply(context.Background(), tt.cli, tt.act) | ||
|
|
||
| // An error here proves that the tx-mode was passed to atlasexec, which |
There was a problem hiding this comment.
I thought we were aiming for tests that use an HTTP server and check that calls were made?
Also, relying on Atlas raw error strings seem fragile to me. These error strings could easily be changed in the Atlas repository and no one would think that this affects this repository
There was a problem hiding this comment.
This test checks a local flow, cloud mode is added in one of the next PRs which will use a local HTTP server.
re the tests fragility (I prefer the Google-y term "brittle", I agree it's not iron-clad. However, there is a trade-off, not relying on any error message (only the exit code) doesn't prove the argument is passed. Relying on mocking on the other hand is less brittle, but has its own costs and risks which can be discussed.
In this case, the risk of the test breaking because of an upstream error message change is low and has no impact on the user, so I opt for the simple way even if it is not 100% textbook (In general, I try to be practical and not religious about "The Right Way™")
| t.Run("baseline", func(t *testing.T) { | ||
| tt := newT(t) | ||
| tt.setInput("url", "sqlite://"+tt.db) | ||
| tt.setInput("dir", "file://testdata/migrations/") |
There was a problem hiding this comment.
nitpicking a bit, but also a honest question since I had an opposite comment about that in one of my PRs - the raw values here are duplicated several times, why not extract them to a const?
There was a problem hiding this comment.
I don't have hard rules about when to use a constant and when to reuse a value. Places where it's more obvious to use a constant are when the value is used again downstream in a comparison in which case the const doesn't only save repetition (tho the constant itself is repeated), it proves correctnes (we compare to the same third value which formally must be equal and not prone to typos).
Another thing to consider is readability. Extracting to a constant sometimes makes the code less readable because you need to jump in the file to understand what that is. In this case, I think its important because the reader can understand which directory I'm referring to. It is very likely that this test func will have many cases with different migration directories to show different use cases.
| act.SetOutput("current", run.Current) | ||
| act.SetOutput("target", run.Target) | ||
| act.SetOutput("pending_count", strconv.Itoa(len(run.Pending))) | ||
| act.SetOutput("applied_count", strconv.Itoa(len(run.Applied))) |
There was a problem hiding this comment.
What are all these outputs? Don’t we have a way to run atlasexec with standard Atlas output?
No description provided.