Skip to content

Conversation

@akrem-chabchoub
Copy link
Contributor

@akrem-chabchoub akrem-chabchoub commented Dec 1, 2025

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

Add synctest in controller_test.go

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

#5232

Screenshots (if appropriate):

@akrem-chabchoub akrem-chabchoub force-pushed the migrate-to-synctest-access-control branch from c243af3 to 1a8e92d Compare December 1, 2025 22:30
@akrem-chabchoub akrem-chabchoub changed the title test: use synctest for access control test and scope CI tests to pk… test: integrate synctest in access control pkg Dec 1, 2025
@akrem-chabchoub akrem-chabchoub self-assigned this Dec 1, 2025
@akrem-chabchoub akrem-chabchoub marked this pull request as ready for review December 1, 2025 23:16
Copilot AI review requested due to automatic review settings December 1, 2025 23:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR integrates synctest from the testing/synctest package into the access control test suite to enable deterministic time-based testing. The change wraps a test case that involves time-dependent behavior (adding and revoking access with historical timestamp lookups) in a synctest.Test wrapper.

Key Changes

  • Added testing/synctest import to the test file
  • Wrapped the "add and revoke then get from history" test case with synctest.Test() for controlled time advancement

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +246 to +251
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)
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
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)
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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants