Skip to content

test: integrate synctest in access control pkg #5296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
akrem-chabchoub wants to merge 2 commits into
base: master
Choose a base branch
from
+41 −38
Open

test: integrate synctest in access control pkg #5296

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a8e92d
test: integrate synctest in access control pkg
akrem-chabchoub Dec 1, 2025
d6f6daf
chore: revert makefile test changes
akrem-chabchoub Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 41 additions & 38 deletions pkg/accesscontrol/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"crypto/ecdsa"
"reflect"
"testing"
"testing/synctest"
"time"

"github.com/ethersphere/bee/v2/pkg/accesscontrol"
Expand Down Expand Up @@ -235,44 +236,46 @@ func TestController_UpdateHandler(t *testing.T) {
assert.Len(t, gl.Get(), 2)
})
t.Run("add and revoke then get from history", func(t *testing.T) {
addRevokeList := []*ecdsa.PublicKey{&grantee.PublicKey}
ref := swarm.RandAddress(t)
_, hRef, encRef, err := c.UploadHandler(ctx, ls, ref, &publisher.PublicKey, swarm.ZeroAddress)
require.NoError(t, err)

// Need to wait a second before each update call so that a new history mantaray fork is created for the new key(timestamp) entry
time.Sleep(1 * time.Second)
beforeRevokeTS := time.Now().Unix()
_, egranteeRef, hrefUpdate1, _, err := c.UpdateHandler(ctx, ls, gls, swarm.ZeroAddress, hRef, &publisher.PublicKey, addRevokeList, nil)
require.NoError(t, err)

time.Sleep(1 * time.Second)
granteeRef, _, hrefUpdate2, _, err := c.UpdateHandler(ctx, ls, gls, egranteeRef, hrefUpdate1, &publisher.PublicKey, nil, addRevokeList)
require.NoError(t, err)

gl, err := accesscontrol.NewGranteeListReference(ctx, ls, granteeRef)
require.NoError(t, err)
assert.Empty(t, gl.Get())
// expect history reference to be different after grantee list update
assert.NotEqual(t, hrefUpdate1, hrefUpdate2)

granteeDH := accesscontrol.NewDefaultSession(grantee)
granteeAl := accesscontrol.NewLogic(granteeDH)
granteeCtrl := accesscontrol.NewController(granteeAl)
// download with grantee shall still work with the timestamp before the revoke
decRef, err := granteeCtrl.DownloadHandler(ctx, ls, encRef, &publisher.PublicKey, hrefUpdate2, beforeRevokeTS)
require.NoError(t, err)
assert.Equal(t, ref, decRef)

// download with grantee shall NOT work with the latest timestamp
decRef, err = granteeCtrl.DownloadHandler(ctx, ls, encRef, &publisher.PublicKey, hrefUpdate2, time.Now().Unix())
require.Error(t, err)
assert.Equal(t, swarm.ZeroAddress, decRef)

// publisher shall still be able to download with the timestamp before the revoke
decRef, err = c.DownloadHandler(ctx, ls, encRef, &publisher.PublicKey, hrefUpdate2, beforeRevokeTS)
require.NoError(t, err)
assert.Equal(t, ref, decRef)
synctest.Test(t, func(t *testing.T) {
addRevokeList := []*ecdsa.PublicKey{&grantee.PublicKey}
ref := swarm.RandAddress(t)
_, hRef, encRef, err := c.UploadHandler(ctx, ls, ref, &publisher.PublicKey, swarm.ZeroAddress)
require.NoError(t, err)

// Need to wait a second before each update call so that a new history mantaray fork is created for the new key(timestamp) entry
time.Sleep(1 * time.Second)
beforeRevokeTS := time.Now().Unix()
_, egranteeRef, hrefUpdate1, _, err := c.UpdateHandler(ctx, ls, gls, swarm.ZeroAddress, hRef, &publisher.PublicKey, addRevokeList, nil)
require.NoError(t, err)

time.Sleep(1 * time.Second)
Comment on lines +246 to +251
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using synctest.Test, you should replace time.Sleep() calls with synctest.Wait() to properly utilize synctest's controlled time advancement. The current implementation wraps the test with synctest but still uses time.Sleep(), which defeats the purpose of using synctest.

Replace:

time.Sleep(1 * time.Second)

With:

synctest.Wait()

This pattern is used consistently in other parts of the codebase (e.g., pkg/blocker/blocker_test.go).

Suggested change
time.Sleep(1 * time.Second)
beforeRevokeTS := time.Now().Unix()
_, egranteeRef, hrefUpdate1, _, err := c.UpdateHandler(ctx, ls, gls, swarm.ZeroAddress, hRef, &publisher.PublicKey, addRevokeList, nil)
require.NoError(t, err)
time.Sleep(1 * time.Second)
synctest.Wait()
beforeRevokeTS := time.Now().Unix()
_, egranteeRef, hrefUpdate1, _, err := c.UpdateHandler(ctx, ls, gls, swarm.ZeroAddress, hRef, &publisher.PublicKey, addRevokeList, nil)
require.NoError(t, err)
synctest.Wait()

Copilot uses AI. Check for mistakes.
Comment on lines +246 to +251
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using synctest.Test, you should replace time.Sleep() calls with synctest.Wait() to properly utilize synctest's controlled time advancement. The current implementation wraps the test with synctest but still uses time.Sleep(), which defeats the purpose of using synctest.

Replace:

time.Sleep(1 * time.Second)

With:

synctest.Wait()

This pattern is used consistently in other parts of the codebase (e.g., pkg/blocker/blocker_test.go).

Suggested change
time.Sleep(1 * time.Second)
beforeRevokeTS := time.Now().Unix()
_, egranteeRef, hrefUpdate1, _, err := c.UpdateHandler(ctx, ls, gls, swarm.ZeroAddress, hRef, &publisher.PublicKey, addRevokeList, nil)
require.NoError(t, err)
time.Sleep(1 * time.Second)
synctest.Wait()
beforeRevokeTS := time.Now().Unix()
_, egranteeRef, hrefUpdate1, _, err := c.UpdateHandler(ctx, ls, gls, swarm.ZeroAddress, hRef, &publisher.PublicKey, addRevokeList, nil)
require.NoError(t, err)
synctest.Wait()

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@akrem-chabchoub akrem-chabchoub Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

synctest.Wait() doesn't advance time, it only waits until all goroutines are blocked according to the official docs.
This test needs the virtual clock to actually advance so i kept the time.Sleep(). The comment on line 245 explains why.
So without time.Sleep() in this test, time.Now() returns the same value twice, causing an error.

granteeRef, _, hrefUpdate2, _, err := c.UpdateHandler(ctx, ls, gls, egranteeRef, hrefUpdate1, &publisher.PublicKey, nil, addRevokeList)
require.NoError(t, err)

gl, err := accesscontrol.NewGranteeListReference(ctx, ls, granteeRef)
require.NoError(t, err)
assert.Empty(t, gl.Get())
// expect history reference to be different after grantee list update
assert.NotEqual(t, hrefUpdate1, hrefUpdate2)

granteeDH := accesscontrol.NewDefaultSession(grantee)
granteeAl := accesscontrol.NewLogic(granteeDH)
granteeCtrl := accesscontrol.NewController(granteeAl)
// download with grantee shall still work with the timestamp before the revoke
decRef, err := granteeCtrl.DownloadHandler(ctx, ls, encRef, &publisher.PublicKey, hrefUpdate2, beforeRevokeTS)
require.NoError(t, err)
assert.Equal(t, ref, decRef)

// download with grantee shall NOT work with the latest timestamp
decRef, err = granteeCtrl.DownloadHandler(ctx, ls, encRef, &publisher.PublicKey, hrefUpdate2, time.Now().Unix())
require.Error(t, err)
assert.Equal(t, swarm.ZeroAddress, decRef)

// publisher shall still be able to download with the timestamp before the revoke
decRef, err = c.DownloadHandler(ctx, ls, encRef, &publisher.PublicKey, hrefUpdate2, beforeRevokeTS)
require.NoError(t, err)
assert.Equal(t, ref, decRef)
})
})
t.Run("add twice", func(t *testing.T) {
addList := []*ecdsa.PublicKey{&grantee.PublicKey, &grantee.PublicKey}
Expand Down
Loading