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

Fix SessionDestroyer test error undefined sid #152

Closed
wants to merge 38 commits into from
Closed

Fix SessionDestroyer test error undefined sid #152

wants to merge 38 commits into from

Conversation

yindia
Copy link
Contributor

@yindia yindia commented Dec 12, 2019

Related issue

It will fix the SessionDestroyer test error undefined sid which is created from #139

Proposed changes

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
    vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate)

Further comments

Yuvraj and others added 30 commits December 11, 2019 00:31
Testcase Added for SessionDestroyer
Testcase updated for SessionDestroyer
Testcase added for DeleteSessionFor
Testcase updated for DeleteSessionFor
Fix syntax error for testcases SessionDestroyer & DeleteSessionsFor
Added new post hook in registry_default_hooks
@claassistantio
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ evalsocket
❌ Yuvraj


Yuvraj seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

aeneasr added a commit that referenced this pull request Dec 12, 2019
Due to a bug in `go-acc`, tests would not run if `-tags sqlite` was supplied as a go tool argument to `go-acc`. This patch resolves that issue and also includes several test patches from previous community PRs and some internal test issues.

Closes #152
Closes #151
@aeneasr aeneasr closed this in #154 Dec 12, 2019
aeneasr added a commit that referenced this pull request Dec 12, 2019
Due to a bug in `go-acc`, tests would not run if `-tags sqlite` was supplied as a go tool argument to `go-acc`. This patch resolves that issue and also includes several test patches from previous community PRs and some internal test issues.

Closes #152
Closes #151
@aeneasr
Copy link
Member

aeneasr commented Dec 12, 2019

Thank you for trying to resolve these issues!. I've pushed up a PR that also includes fixes for these issues!

Regarding git workflow, you should never work on one branch. I recommend reading: https://hackernoon.com/high-velocity-software-development-using-nested-git-branches-69bfc2d1e54d

There are also other issues in the code, for example a duplicate variable declaration which causes a build failure: https://github.com/ory/kratos/pull/152/files#diff-4c06363fefe733cfec759b8c34d2048fL45

The best way to ensure that your stuff actually works is by running tests, for example using go test ./... -tags sqlite.

I'm closing this because it superseded by #154

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