script: support -scripttest.update in db/cmp#151
Conversation
|
|
||
| case <-timeoutChan: | ||
| if s.DoUpdate { | ||
| s.FileUpdates[args[1]] = lastActual |
There was a problem hiding this comment.
let's add filename := args[1] to where we read the file and use filename after that instead of looking into args.
I also wonder now if we'd need to normalize the name that we put into FileUpdates. In scripttest it's looking for files that exactly match what we put into FileUpdates so this might fail in weird ways if we do e.g. db/cmp foo ./foo.table. If you've got cycles could you try out and see if we've got edge case like that? We'd need fix in cilium/hive too for cmp... Though I don't think this is a big deal to leave it like this.
There was a problem hiding this comment.
Added fileName := args[1] (the camelCase spelling looked better next to tableName). Will look into whether we need to normalize names as a follow-up when I have some cycles...
| @@ -409,6 +414,7 @@ func CompareCmd(db *DB) script.Cmd { | |||
| fmt.Fprintf(w, "+ %s\n", line) | |||
There was a problem hiding this comment.
BTW, the diff output is a bit broken in that if there's an extra row it'll end up doing -/+ for every row after as they're offset. Might maybe make sense to look into either fixing this or using a proper diff library... in case you've got cycles and want to work on improving this also 😄
There was a problem hiding this comment.
Yeah, I noticed that too on some occasions 😄 Will add this to my TODO list, would be something nice to look into as part of my 30% time.
| @@ -419,6 +425,10 @@ func CompareCmd(db *DB) script.Cmd { | |||
| return nil, s.Context().Err() | |||
|
|
|||
| case <-timeoutChan: | |||
There was a problem hiding this comment.
Hmm do we have issues if the timeout here is almost as large as the timeout for the whole test? We might end up timing out the test even with -scripttest.update. The default db/cmp timeout is 5 seconds and often the script tests have a 10 second timeout, so you'd probably manage to update one db/cmp output but then timeout on the second.
It is tricky though as you don't want to take the immediate contents of the table because it might not have yet reconciled so you do want to wait and try few times.
Maybe we'll just add this in in this form and just see what issues we might run into and adapt then.
There was a problem hiding this comment.
Hmm do we have issues if the timeout here is almost as large as the timeout for the whole test? We might end up timing out the test even with -scripttest.update. The default db/cmp timeout is 5 seconds and often the script tests have a 10 second timeout, so you'd probably manage to update one db/cmp output but then timeout on the second.
Hmmm, good point. Yeah, this sounds tricky to get right.
I guess with the current state we could at least run with -scripttest.update multiple times to also update successive outputs if we timed out and only updated the first one.
Maybe we'll just add this in in this form and just see what issues we might run into and adapt then.
👍 I'd be inclined to do that.
Hmm not sure what's going on with this again. Tests pass otherwise so not a blocker for this PR. |
Make db/cmp populate script.State.FileUpdates with the observed table output when s.DoUpdate is set, so .txtar golden table sections can be automatically rewritten on -scripttest.update, akin to how cmp already does. Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser
force-pushed
the
pr/tklauser/db-cmp-update
branch
from
a93a333 to
57ff262
Compare
According to the Let me try that in a separate PR: #152 |
We started seeing the error of the pr workflow [^1]:
Run thollander/actions-comment-pull-request@v2
No comment has been found with asked pattern. Creating a new comment.
Error: Resource not accessible by integration
According to the thollander/actions-comment-pull-request action's
README[^2] we need to add pull-requestes: write permissions to the
workflow.
Do that and also update the action to v3 while at it.
[^1]: #151 (comment)
[^2]: https://github.com/thollander/actions-comment-pull-request/blob/e4a76dd2b0a3c2027c3fd84147a67c22ee4c90fa/README.md?plain=1#L172-L181
Signed-off-by: Tobias Klauser <tobias@cilium.io>
We started seeing the error of the pr workflow [^1]:
Run thollander/actions-comment-pull-request@v2
No comment has been found with asked pattern. Creating a new comment.
Error: Resource not accessible by integration
According to the thollander/actions-comment-pull-request action's
README[^2] we need to add pull-requestes: write permissions to the
workflow.
Do that and also update the action to v3 while at it.
[^1]: #151 (comment)
[^2]: https://github.com/thollander/actions-comment-pull-request/blob/e4a76dd2b0a3c2027c3fd84147a67c22ee4c90fa/README.md?plain=1#L172-L181
Signed-off-by: Tobias Klauser <tobias@cilium.io>
We started seeing the error of the pr workflow [^1]:
Run thollander/actions-comment-pull-request@v2
No comment has been found with asked pattern. Creating a new comment.
Error: Resource not accessible by integration
According to the thollander/actions-comment-pull-request action's
README[^2] we need to add pull-requestes: write permissions to the
workflow.
Do that and also update the action to v3 while at it.
[^1]: #151 (comment)
[^2]: https://github.com/thollander/actions-comment-pull-request/blob/e4a76dd2b0a3c2027c3fd84147a67c22ee4c90fa/README.md?plain=1#L172-L181
Signed-off-by: Tobias Klauser <tobias@cilium.io>
We started seeing the error of the pr workflow [^1]:
Run thollander/actions-comment-pull-request@v2
No comment has been found with asked pattern. Creating a new comment.
Error: Resource not accessible by integration
According to the thollander/actions-comment-pull-request action's
README[^2] we need to add pull-requestes: write permissions to the
workflow.
Do that and also update the action to v3 while at it.
[^1]: #151 (comment)
[^2]: https://github.com/thollander/actions-comment-pull-request/blob/e4a76dd2b0a3c2027c3fd84147a67c22ee4c90fa/README.md?plain=1#L172-L181
Signed-off-by: Tobias Klauser <tobias@cilium.io>
We started seeing the error of the pr workflow [^1]:
Run thollander/actions-comment-pull-request@v2
No comment has been found with asked pattern. Creating a new comment.
Error: Resource not accessible by integration
According to the thollander/actions-comment-pull-request action's
README[^2] we need to add pull-requestes: write permissions to the
workflow.
Do that and also update the action to v3 while at it.
[^1]: #151 (comment)
[^2]: https://github.com/thollander/actions-comment-pull-request/blob/e4a76dd2b0a3c2027c3fd84147a67c22ee4c90fa/README.md?plain=1#L172-L181
Signed-off-by: Tobias Klauser <tobias@cilium.io>
2 participants
Make
db/cmppopulatescript.State.FileUpdateswith the observed table output whens.DoUpdateis set, so.txtargolden table sections can be automatically rewritten on-scripttest.update, akin to howcmpalready does.