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

add some tests #7

Merged
merged 1 commit into from
Jul 18, 2019
Merged

add some tests #7

merged 1 commit into from
Jul 18, 2019

Conversation

xcaptain
Copy link
Collaborator

@xcaptain xcaptain commented Jul 17, 2019

Still work in progress, test cases are copied from Casbin.NET project, in the future more tests should be added.

@xcaptain xcaptain mentioned this pull request Jul 17, 2019
9 tasks
Copy link
Member

@hsluoyz hsluoyz left a comment

Choose a reason for hiding this comment

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

Please fix.

.vscode/launch.json Outdated Show resolved Hide resolved
@xcaptain
Copy link
Collaborator Author

xcaptain commented Jul 17, 2019

@hsluoyz do you think we should implement all the test cases from Casbin.NET or just some? also how about we move this project into Casbin.NET because this project is so simple and share a lot of tests with Casbin.NET

@hsluoyz
Copy link
Member

hsluoyz commented Jul 17, 2019

If your adapter supports AutoSave, then you should follow: https://github.com/casbin/xorm-adapter/blob/master/adapter_test.go. If not, then follow: https://github.com/casbin/cassandra-adapter/blob/master/adapter_test.go

I don't think moving into Casbin is a good idea. Because all adapters have their own repo.

And for the commits in this PR, please merge them into one single commit, for easier review.

@xcaptain
Copy link
Collaborator Author

@hsluoyz I think this adapter is using autosave and I can't think of any reason to disable autosave. By default DbContext in an asp dotnet core project is injected at scoped service, this adapter depends on DbContext, so its lifetime must be scoped or transient, which means this adapter will be created per requests, which means write or read from database per request.

Thanks for pointing out how to test an adapter, I will follow your xorm-adapter project.

@xcaptain
Copy link
Collaborator Author

xcaptain commented Jul 17, 2019

@hsluoyz Please take a look, I follow testAutoSave in xorm-adapter, the test cases passed in my laptop. I will rebase these commits before merge.

@hsluoyz
Copy link
Member

hsluoyz commented Jul 18, 2019

lgtm. Please squash the commits.

@xcaptain
Copy link
Collaborator Author

rebased

@hsluoyz hsluoyz merged commit a3f8b86 into master Jul 18, 2019
@hsluoyz
Copy link
Member

hsluoyz commented Jul 18, 2019

Merged.

@hsluoyz
Copy link
Member

hsluoyz commented Jul 18, 2019

Hi @xcaptain , next step, please add the CI and code coverage badges.

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.

2 participants