1
0
Fork
You've already forked forgejo
0
forked from forgejo/forgejo

Review 2 of forgejo/forgejo!12512 #2

Closed
oliverpool wants to merge 12 commits from oliverpool/forgejo:api-test-review into wip-api-repo-access
pull from: oliverpool/forgejo:api-test-review
merge into: limiting-factor:wip-api-repo-access
limiting-factor:forgejo
limiting-factor:wip-permissions-howto
limiting-factor:wip-permissions-testCases
limiting-factor:wip-5445-config-docs
limiting-factor:wip-cache-test
limiting-factor:wip-coverage
limiting-factor:wip-f3-driver-new
limiting-factor:wip-308-logfile
limiting-factor:wip-12700-project-permissions
limiting-factor:wip-checkForkDestination
limiting-factor:wip-api-context-accessors-v15
limiting-factor:wip-unify-api-context-getter
limiting-factor:wip-api-context-accessors-v15-save
limiting-factor:wip-reactions-notifications
limiting-factor:wip-permissions-documentation
limiting-factor:wip-api-context-accessors
limiting-factor:wip-requiredScopeCategories
limiting-factor:wip-api-context-accessors-save
limiting-factor:wip-coverage-redis
limiting-factor:wip-api-repo-access-v15
limiting-factor:wip-integration-permissions
limiting-factor:wip-api-repo-access
limiting-factor:wip-comment-ghost
limiting-factor:renovate/forgejo-patch-vue-monorepo
limiting-factor:renovate/forgejo-github.com-urfave-cli-v3-3.x
limiting-factor:renovate/forgejo-codemirror
limiting-factor:renovate/forgejo-go-openapi
limiting-factor:renovate/forgejo-forgejoforgejo-build-publish
limiting-factor:bp-v15.0/forgejo-b1b47e6
limiting-factor:renovate/forgejo-github.com-mattn-go-sqlite3-1.x
limiting-factor:renovate/forgejo-github.com-go-swagger-go-swagger-cmd-swagger-0.x
limiting-factor:v15.0/forgejo
limiting-factor:renovate/forgejo-github.com-sourcegraph-zoekt-digest
limiting-factor:v11.0/forgejo
limiting-factor:renovate/forgejo-github.com-golangci-golangci-lint-v2-cmd-golangci-lint-2.x
limiting-factor:renovate/forgejo-github.com-alecthomas-chroma-v2-2.x
limiting-factor:renovate/forgejo-github.com-go-chi-chi-v5-5.x
limiting-factor:wip-trust-v15
limiting-factor:wip-trust-team
limiting-factor:bp-v15.0/forgejo-5c61808
limiting-factor:wip-api-test
limiting-factor:bp-v15.0/forgejo-6cd3f02
limiting-factor:wip-approval-v15
limiting-factor:wip-approval
limiting-factor:wip-f3-driver
limiting-factor:v14.0/forgejo
limiting-factor:wip-buffer-logger
limiting-factor:bp-v15.0/forgejo-2176403
limiting-factor:wip-teams
limiting-factor:bp-v14.0/forgejo-9767ceb
limiting-factor:bp-v14.0/forgejo-d996dfb
limiting-factor:v13.0/forgejo
limiting-factor:bp-v13.0/forgejo-dc0a63e
limiting-factor:bp-v13.0/forgejo-e7ef2eb
limiting-factor:v12.0/forgejo
limiting-factor:bp-v12.0/forgejo-cf1fda8-a511e37
limiting-factor:bp-v11.0/forgejo-b52cec7
limiting-factor:v7.0/forgejo
limiting-factor:v10.0/forgejo
limiting-factor:v9.0/forgejo
limiting-factor:v8.0/forgejo
limiting-factor:v1.21/forgejo
limiting-factor:v1.20/forgejo
limiting-factor:v1.19/forgejo
limiting-factor:v1.18/forgejo
limiting-factor:v1.17/forgejo
Contributor
Copy link

Review of forgejo/forgejo!12512.

As last time, feel free to pick/ignore what you want!

The main change is getting rid of the reflect import:

  • since most middleware have no additional argument, I changed the signature of registerFunctionTest
  • the other middlewares must arrange the arguments themselves (using type assertion, because they know the function to call and the expected types)

This should make debugging easier, but it looses the verbose logging (each middleware should call t.Logf if we wanted to restore this behavior).

I also simplified the protect field, since only booleans are protected.

Do you think that it would be achievable to make the fixtureData strongly typed? (instead of a map[string]string). It might be too much effort (and might need 2 types: one to express what is needed and one to pass around the created models).

Review of forgejo/forgejo!12512. As last time, feel free to pick/ignore what you want! --- The main change is getting rid of the `reflect` import: - since most middleware have no additional argument, I changed the signature of `registerFunctionTest` - the other middlewares must arrange the arguments themselves (using type assertion, because they know the function to call and the expected types) This should make debugging easier, but it looses the verbose logging (each middleware should call `t.Logf` if we wanted to restore this behavior). --- I also simplified the `protect` field, since only booleans are protected. --- Do you think that it would be achievable to make the `fixtureData` strongly typed? (instead of a `map[string]string`). It might be too much effort (and might need 2 types: one to express what is needed and one to pass around the created models). 
We lose the type-parametrization, but fortunately all protects are
called on boolean. If we want flexibility, we could expose a
registerCleanup(t *testingT) field instead.
oliverpool left a comment
Copy link

As previously, I reviewed and tested some of my ideas locally. Here is my brain-dump!

After playing more with it, I think I understand how it operates. Feel free to ping/harass me if I let too much time pass while you wish a review for this PR ❤️

As previously, I reviewed and tested some of my ideas locally. Here is my brain-dump! After playing more with it, I think I understand how it operates. Feel free to ping/harass me if I let too much time pass while you wish a review for this PR ❤️
@ -611,1 +611,3 @@
protectfunc()func()
// List the settings which might be updated while interpreting the fixtureData
// so that they are restored upon test completion.
protectSettingsBool[]*bool
Author
Contributor
Copy link

We lose the type-parametrization, but fortunately all protects are
called on boolean. If we want flexibility, we could expose a
registerCleanup(t *testingT) field instead.

We lose the type-parametrization, but fortunately all protects are called on boolean. If we want flexibility, we could expose a registerCleanup(t *testingT) field instead.
limiting-factor marked this conversation as resolved
@ -623,2 +626,4 @@
builder(t,signatureString,signature)
}elseifsignatureString==prefix{
panic("unreachable")// strings.HasPrefix already covers this case
}
Author
Contributor
Copy link

the 2 lines above can be deleted :)

the 2 lines above can be deleted :)

removed, good catch

removed, good catch
limiting-factor marked this conversation as resolved
@ -630,1 +634,3 @@
signatureString:=apiv1_permissions_tests.SignatureToString(signature)
funcregisterFunctionTest(signaturefunc(apiv1_permissions.Context),testfunctionTest)bool{
test.call=func(_*testing.T,ctxapiv1_permissions.Context,_*apiv1_permissions.Permissions,_*fixtureData,_[]any){
signature(ctx)
Author
Contributor
Copy link

Adding a t.Logf("calling ..." would be easy here (but would need one in all other places where call is defined)

Adding a `t.Logf("calling ..."` would be easy here (but would need one in all other places where `call` is defined)

Done in a followup commit.

Done in a followup commit.
limiting-factor marked this conversation as resolved
@ -63,2 +63,4 @@
},
},
call:func(_*testing.T,ctxapiv1_permissions.Context,_*apiv1_permissions.Permissions,_*fixtureData,args[]any){
apiv1_permissions.ReqRepoWriter(ctx,args[0].([]unit_model.Type))
Author
Contributor
Copy link

I find this easier to understand (and hope that it is correct 👼)

I find this easier to understand (and hope that it is correct 👼)

It is correct.

It is correct.

And easier to read too.

And easier to read too.
limiting-factor marked this conversation as resolved

@oliverpool wrote in #2 (comment):

Feel free to ping/harass me if I let too much time pass while you wish a review for this PR ❤️

I will keep that in mind and gently ping. It is also fine for you to say stop if this is too much at any point, of course.

@oliverpool wrote in https://codeberg.org/limiting-factor/forgejo/pulls/2#issuecomment-17368454: > Feel free to ping/harass me if I let too much time pass while you wish a review for this PR :heart: I will keep that in mind and gently ping. It is also fine for you to say stop if this is too much at any point, of course.

@oliverpool wrote in #2 (comment):

Review of forgejo/forgejo!12512.

As last time, feel free to pick/ignore what you want!

Thank you. I love that review methodology.

The main change is getting rid of the reflect import:

* since most middleware have no additional argument, I changed the signature of `registerFunctionTest`
* the other middlewares must arrange the arguments themselves (using type assertion, because they know the function to call and the expected types)

This should make debugging easier, but it looses the verbose logging (each middleware should call t.Logf if we wanted to restore this behavior).

I see how this makes the code easier to read. No need to figure out how args relates to the function. It reduces the complexity. I'll take that as is, it works fine. And I'll:

  • update the README
  • add the suggested Logf

For your entertainment, you will find below the message I started to write to object that it was not such a great idea. Until I realized it was.

I think it also makes it harder to debug and easier to get wrong. Restoring the debug output that currently looks like:

 functions_test.go:171: calling permissions.TokenRequiresScopes(ctx, auth.AccessTokenScopeLevel(1))
 functions_test.go:148: 	+ []auth.AccessTokenScopeCategory([7])

can be done with a Logf as you suggest but it will have to be done manually with all the challenges to consistency it involves.

In the current code, if the args function fails to provide what is expected, the sanity check of prepareCall (length) and the error messages provided by fn.Call are (IMHO) rather helpful in figuring out what went wrong.

Knowing that the call and its arguments (calling permissions.TokenRequiresScopes(ctx, auth.AccessTokenScopeLevel(1))) are not subject to an error in the test being written helps focus the investigation on how the arguments are incorrectly built.

@oliverpool wrote in https://codeberg.org/limiting-factor/forgejo/pulls/2#issue-5791514: > Review of forgejo/forgejo!12512. > > As last time, feel free to pick/ignore what you want! Thank you. I love that review methodology. > The main change is getting rid of the `reflect` import: > > * since most middleware have no additional argument, I changed the signature of `registerFunctionTest` > > * the other middlewares must arrange the arguments themselves (using type assertion, because they know the function to call and the expected types) > > > This should make debugging easier, but it looses the verbose logging (each middleware should call `t.Logf` if we wanted to restore this behavior). > I see how this makes the code easier to read. No need to figure out how args relates to the function. It reduces the complexity. I'll take that as is, it works fine. And I'll: - update the README - add the suggested Logf --- For your entertainment, you will find below the message I started to write to object that it was not such a great idea. Until I realized it was. <details> I think it also makes it harder to debug and easier to get wrong. Restoring the debug output that currently looks like: ``` functions_test.go:171: calling permissions.TokenRequiresScopes(ctx, auth.AccessTokenScopeLevel(1)) functions_test.go:148: + []auth.AccessTokenScopeCategory([7]) ``` can be done with a `Logf` as you suggest but it will have to be done manually with all the challenges to consistency it involves. In the current code, if the args function fails to provide what is expected, the sanity check of `prepareCall` (length) and the error messages provided by `fn.Call` are (IMHO) rather helpful in figuring out what went wrong. Knowing that the call and its arguments (`calling permissions.TokenRequiresScopes(ctx, auth.AccessTokenScopeLevel(1))`) are not subject to an error in the test being written helps focus the investigation on how the arguments are incorrectly built. </details>

@oliverpool wrote in #2 (comment):

I also simplified the protect field, since only booleans are protected.

Good idea 👍

@oliverpool wrote in https://codeberg.org/limiting-factor/forgejo/pulls/2#issue-5791514: > I also simplified the `protect` field, since only booleans are protected. Good idea 👍

@oliverpool wrote in #2 (comment):

Do you think that it would be achievable to make the fixtureData strongly typed? (instead of a map[string]string). It might be too much effort (and might need 2 types: one to express what is needed and one to pass around the created models).

I'm not sure it is worth the effort. I expect those to mutate significantly and the flexibility of untyped / strings help. Having strong types would add friction and I'm not sure which types would make sense. The code triggered by a given value is quite diverse and the way they combine with each other is often ad-hoc. 🤔

@oliverpool wrote in https://codeberg.org/limiting-factor/forgejo/pulls/2#issue-5791514: > Do you think that it would be achievable to make the `fixtureData` strongly typed? (instead of a `map[string]string`). It might be too much effort (and might need 2 types: one to express what is needed and one to pass around the created models). I'm not sure it is worth the effort. I expect those to mutate significantly and the flexibility of untyped / strings help. Having strong types would add friction and I'm not sure which types would make sense. The code triggered by a given value is quite diverse and the way they combine with each other is often ad-hoc. 🤔

cherry-picked manually, as they were.

cherry-picked manually, as they were.

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
limiting-factor/forgejo!2
Reference in a new issue
limiting-factor/forgejo
No description provided.
Delete branch "oliverpool/forgejo:api-test-review"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?