-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
[WorkInProgress] Add two failing policies and equality check #106
Conversation
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
…nto WorkInProgress Merging for signature change
@@ -147,6 +148,23 @@ var TestManagerPolicies = []*DefaultPolicy{ | |||
"owner": &EqualsSubjectCondition{}, | |||
}, | |||
}, | |||
//Two new policies which do not persist in MySQL correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the tests might be a bit confusing here... Anything that is in TestManagerPolicies
will be added automatically to the manager that is currently being tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, two questions. I changed the assert, so the assert is not failing now, but :
- Why is
go test ./...
not working? I am not able to testmanager_test_helper.go
file locally. - If the policies I added in TestManagerPolicies above are automatically added to the manager that is currently being tested, why are the tests failing now?
ladon.Conditions(nil)
is mentioned in the error trace, but I do not understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is
go test ./...
not working? I am not able to test manager_test_helper.go file locally.
I don't know, make sure to run it from the project root. Add a test that fails for sure (assert.True(false)
) and see if tests fail.
If the policies I added in TestManagerPolicies above are automatically added to the manager that is currently being tested, why are the tests failing now? ladon.Conditions(nil) is mentioned in the error trace, but I do not understand.
You omitted the conditions which causes them to be of nil value but it the manager never returns conditions of type nil in order to prevent invalid pointer panics. If you look at the other policies you'll notice that each policy sets Conditions: Conditions{},
which you omitted
Here, tests fail - see https://travis-ci.org/ory/ladon/builds/337514939#L590 |
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
manager_test_helper.go
Outdated
found[got.GetID()]++ | ||
} | ||
} | ||
} | ||
|
||
for _, f := range found { | ||
//This assert is supposed to fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not true, found is a map[string]int{}
which has for every policy id a value of 1 so this can't fail. A better test would be something like this (pseudocode):
found := map[string]int{}
for _, got := range pols {
for _, expect := range TestManagerPolicies {
if got.GetID() == expect.GetID() {
DeepEquals(expect, got)
found[got.GetID()]++
}
}
}
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
…nto WorkInProgress Simple merge
I added the conditions fields and the tests passed. I meant to keep the assert that way and forgot to change the comment. So here, I am comparing the resources and actions fields of the policies in the equality check to see that it is persisted correctly. Since the assert is passing in this case, it implies that the policies are persisted in the database correctly, right? |
manager_test_helper.go
Outdated
@@ -354,13 +374,15 @@ func TestHelperCreateGetDelete(s Manager) func(t *testing.T) { | |||
found := map[string]int{} | |||
for _, got := range pols { | |||
for _, expect := range TestManagerPolicies { | |||
if got.GetID() == expect.GetID() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please replace this with
for _, got := range pols {
for _, expect := range TestManagerPolicies {
if got.GetID() == expect.GetID() {
assert.True(t, reflect.DeepEquals(expect, got))
found[got.GetID()]++
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adhicoder please do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still failing. Not sure why though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok, and was also the whole point of this PR (creating a sane failing test to find the bug you described). However, the output is still not conclusive enough (https://travis-ci.org/ory/ladon/builds/338007648#L605). Let's change assert.True(t, reflect.DeepEquals(expect, got))
to assert.ObjectsAreEqualValues(t, expect, got)
which will print a better error message
manager_test_helper.go
Outdated
@@ -354,13 +374,15 @@ func TestHelperCreateGetDelete(s Manager) func(t *testing.T) { | |||
found := map[string]int{} | |||
for _, got := range pols { | |||
for _, expect := range TestManagerPolicies { | |||
if got.GetID() == expect.GetID() { | |||
//This is a modified equality check | |||
if got.GetID() == expect.GetID() && reflect.DeepEqual(got.GetResources(), expect.GetResources()) && reflect.DeepEqual(got.GetActions(), expect.GetActions()) { | |||
found[got.GetID()]++ | |||
} | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add assert.Len(t, found, len(TestManagerPolicies))
here
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
I added the assert. Build fails saying map should have 14 items but has 0. |
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
Paradoxically, the checks have now passed |
Haha, that is very interesting. It sort of supports my suspicion that the bug is somewhere else (because apparently, the bug does not appear with proper tests). Another possibility is that the bug is with the MySQL version. You should change the version here from |
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
So it seems like tests are passing with 5.7.2 as well - so something must be wrong in your usage or set up of ladon, sorry! Thank you for the PR though, I'll add your changes because they are quite valuable! The only thing is to please revert the last change (fc270fd) because I want this library to be compatible with MySQL 5.6 If you want, you can share some the code you had issues with with me, maybe I can spot if something's wrong. |
Signed-off-by: Adhithya Ravindra <Adhithya.Ravindra@gmail.com>
Thank you, I have reverted the changes to the SQL version. I shall share the code that I had issues with soon, please see if you can spot the error. |
Thank you! I will! Please post the code to the original issue :) |
No description provided.