Skip to content
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.

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

test(i): Refactor tests to avoid asserting policyIDs #3171

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 28 additions & 5 deletions tests/integration/acp.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"os"
"os/exec"
"path/filepath"
"slices"
"strings"
"sync/atomic"
"time"
Expand Down Expand Up @@ -91,7 +92,11 @@ type AddPolicy struct {
Identity immutable.Option[identityRef]

// The expected policyID generated based on the Policy loaded in to the ACP system.
ExpectedPolicyID string
//
// This is an optional attribute, for situations where a test might want to assert
// the exact policyID. When this is not provided the test will just assert that
// the resulting policyID is not empty.
ExpectedPolicyID immutable.Option[string]

// Any error expected from the action. Optional.
//
Expand All @@ -105,22 +110,40 @@ func addPolicyACP(
s *state,
action AddPolicy,
) {
// If we expect an error, then ExpectedPolicyID should be empty.
if action.ExpectedError != "" && action.ExpectedPolicyID != "" {
// If we expect an error, then ExpectedPolicyID should never be provided.
if action.ExpectedError != "" && !action.ExpectedPolicyID.HasValue() {
require.Fail(s.t, "Expected error should not have an expected policyID with it.", s.testCase.Description)
}

nodeIDs, nodes := getNodesWithIDs(action.NodeID, s.nodes)
maxNodeID := slices.Max(nodeIDs)
policyIDsAddedSoFar := len(s.policyIDs)
// Expand the policyIDs slice once, so we can minimize how many times we need to expaind it.
// We use the maximum nodeID provided to make sure policyIDs slice can accomodate upto that nodeID.
if policyIDsAddedSoFar <= maxNodeID {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: would prefer if len(s.policyIDs) <= maxNodeID as the variable doesn't add any clarity.

// Expand the slice if required, so that the policyID can be accessed by node index
policyIDs := make([][]string, maxNodeID+1)
copy(policyIDs, s.policyIDs)
s.policyIDs = policyIDs
}

for index, node := range nodes {
ctx := getContextWithIdentity(s.ctx, s, action.Identity, nodeIDs[index])
nodeID := nodeIDs[index]
ctx := getContextWithIdentity(s.ctx, s, action.Identity, nodeID)
policyResult, err := node.AddPolicy(ctx, action.Policy)

expectedErrorRaised := AssertError(s.t, s.testCase.Description, err, action.ExpectedError)
assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised)

if !expectedErrorRaised {
require.Equal(s.t, action.ExpectedError, "")
require.Equal(s.t, action.ExpectedPolicyID, policyResult.PolicyID)
if action.ExpectedPolicyID.HasValue() {
require.Equal(s.t, action.ExpectedPolicyID.Value(), policyResult.PolicyID)
} else {
require.NotEqual(s.t, policyResult.PolicyID, "")
}

s.policyIDs[nodeID] = append(s.policyIDs[nodeID], policyResult.PolicyID)
}

// The policy should only be added to a SourceHub chain once - there is no need to loop through
Expand Down
137 changes: 137 additions & 0 deletions tests/integration/acp/demo/demo_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
// Copyright 2024 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package test_acp_demo

import (
"testing"

"github.com/sourcenetwork/immutable"

testUtils "github.com/sourcenetwork/defradb/tests/integration"
)

func TestACP_DEMO(t *testing.T) {
test := testUtils.TestCase{

Description: "DEMO",

Actions: []any{
testUtils.AddPolicy{ // Specified and subbed in for User1 and User2 schema below

Identity: immutable.Some(1),

Check failure on line 29 in tests/integration/acp/demo/demo_test.go

View workflow job for this annotation

GitHub Actions / Test os job (macos-latest)

cannot use immutable.Some(1) (value of type immutable.Option[int]) as immutable.Option[tests.identityRef] value in struct literal

Policy: `
name: test
description: a test policy which marks a collection in a database as a resource

actor:
name: actor

resources:
users:
permissions:
read:
expr: owner + reader
write:
expr: owner

relations:
owner:
types:
- actor
reader:
types:
- actor
admin:
manages:
- reader
types:
- actor
`,

// TODO: Remove after draft is approved
// Note: This is also valid now that it is optional, but we don't need to provide it at all.
ExpectedPolicyID: immutable.Some(
"94eb195c0e459aa79e02a1986c7e731c5015721c18a373f2b2a0ed140a04b454",
),
},

testUtils.AddPolicy{ // Specified and subbed in for User3 schema below

Identity: immutable.Some(1),

Check failure on line 69 in tests/integration/acp/demo/demo_test.go

View workflow job for this annotation

GitHub Actions / Test os job (macos-latest)

cannot use immutable.Some(1) (value of type immutable.Option[int]) as immutable.Option[tests.identityRef] value in struct literal

Policy: `
name: test
description: another policy

actor:
name: actor

resources:
users:
permissions:
read:
expr: owner + reader
write:
expr: owner

relations:
owner:
types:
- actor
reader:
types:
- actor
admin:
manages:
- reader
types:
- actor
`,

// TODO: Remove after draft is approved
// Note: No policyID assertion needed
},

testUtils.SchemaUpdate{
Schema: `
type Users1 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}

type User2 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}

type User3 @policy(
id: "%policyID%",
resource: "users"
) {
name: String
age: Int
}
`,

PolicyIDs: immutable.Some([]int{0, 0, 1}),
},
},
}

testUtils.ExecuteTestCase(t, test)
}
4 changes: 4 additions & 0 deletions tests/integration/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ type state struct {
// The seed for the next identity generation. We want identities to be deterministic.
nextIdentityGenSeed int

// Policy IDs, by node index, by policyID index (in the order they were added).
policyIDs [][]string

// Will receive an item once all actions have finished processing.
allActionsDone chan struct{}

Expand Down Expand Up @@ -229,6 +232,7 @@ func newState(
collectionIndexesByRoot: map[uint32]int{},
docIDs: [][]client.DocID{},
cids: map[any]string{},
policyIDs: [][]string{},
indexes: [][][]client.IndexDescription{},
isBench: false,
}
Expand Down
45 changes: 45 additions & 0 deletions tests/integration/test_case.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,51 @@ type SchemaUpdate struct {
// The schema update.
Schema string

// PolicyIDs is an optional list argument which makes it easier to substitute/embed PolicyIDs
// into the schema string where place holder labels ("%policyID%") are at, without us using
// explicit policy identifiers.
//
// Note:
// - The number of placeholder labels (%policyID%) must exactly match the number of policyID
// indexes in this list.
// - The list must contain the policyID Indexes in the same order they are to be substituted in.
// - Can substitute the same policyID at an index, by specifying that index multiple times.
// - The indexes must be valid (correspond to the number of policies added so far).
//
// Example:
//
// Consider we have one policy that was added resulting in the following policyID:
// PolicyID="94eb195c0e459aa79e02a1986c7e731c5015721c18a373f2b2a0ed140a04b454"
//
// Then using this attribute like:
// PolicyIDs: immutable.Some([]int{0, 0}),
//
// On a Schema string like:
// ```
// type Users1 @policy(id: "%policyID%", resource: "users") {
// name: String
// age: Int
// }
//
// type Users2 @policy(id: "%policyID%", resource: "users") {
// name: String
// age: Int
// }
// ```
// The Schema that will be loaded will be this modified one:
// ```
// type Users1 @policy(id: "94eb195c0e459aa79e02a1986c7e731c5015721c18a373f2b2a0ed140a04b454", resource: "users") {
// name: String
// age: Int
// }
//
// type Users2 @policy(id: "94eb195c0e459aa79e02a1986c7e731c5015721c18a373f2b2a0ed140a04b454", resource: "users") {
// name: String
// age: Int
// }
// ```
PolicyIDs immutable.Option[[]int]
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I don't think this being immutable.Option adds anything besides complexity - there appears to be no behavioural differences between immutable.None and an empty slice.

Same goes for Fred's suggestion about decoupling this from policyID.


// Optionally, the expected results.
//
// Each item will be compared individually, if ID, RootID, SchemaVersionID or Fields on the
Expand Down
65 changes: 62 additions & 3 deletions tests/integration/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -1058,9 +1058,68 @@ func updateSchema(
s *state,
action SchemaUpdate,
) {
_, nodes := getNodesWithIDs(action.NodeID, s.nodes)
for _, node := range nodes {
results, err := node.AddSchema(s.ctx, action.Schema)
policyIDPlaceHolderLabel := "%policyID%"

// Do some sanitation checks if PolicyIDs are to be substituted, and error out early if invalid usage.
if action.PolicyIDs.HasValue() {
policyIDIndexes := action.PolicyIDs.Value()
if len(policyIDIndexes) == 0 {
require.Fail(s.t, "Specified policyID substitution but none given.", s.testCase.Description)
}

howManyPlaceHolders := strings.Count(action.Schema, policyIDPlaceHolderLabel)
if howManyPlaceHolders == 0 {
require.Fail(
s.t,
"Can't do substitution because no place holder labels:"+policyIDPlaceHolderLabel,
s.testCase.Description,
)
}

if howManyPlaceHolders != len(policyIDIndexes) {
require.Fail(
s.t,
"Can't do substitution when place holders are not equal to specified indexes",
s.testCase.Description,
)
}

}

nodeIDs, nodes := getNodesWithIDs(action.NodeID, s.nodes)
for index, node := range nodes {
// This schema might be modified if the caller needs some substitution magic done.
var modifiedSchema = action.Schema

// We need to substitute the policyIDs into the `%policyID% place holders.
if action.PolicyIDs.HasValue() {
policyIDIndexes := action.PolicyIDs.Value()
nodeID := nodeIDs[index]

nodesPolicyIDs := s.policyIDs[nodeID]
for _, policyIDIndex := range policyIDIndexes {
// Ensure policy index specified is valid (compared the existing policyIDs) for this node.
if policyIDIndex >= len(nodesPolicyIDs) {
require.Fail(
s.t,
"a policyID index is out of range, number of added policies is smaller",
s.testCase.Description,
)
}

policyID := nodesPolicyIDs[policyIDIndex]

// Now actually perform the substitution as all the sanity checks are done.
modifiedSchema = strings.Replace(
modifiedSchema,
policyIDPlaceHolderLabel,
policyID,
1,
)
}
}

results, err := node.AddSchema(s.ctx, modifiedSchema)
expectedErrorRaised := AssertError(s.t, s.testCase.Description, err, action.ExpectedError)

assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised)
Expand Down
Loading