-
Notifications
You must be signed in to change notification settings - Fork 49
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: Add Tokenserver integration tests #1152
Conversation
I'll note this needed an additional
I'm seeing some errors here. Should I have set TOKENSERVER_DATABASE_* to point to the same db as the running old tokenserver's db?
|
@pjenvey I have
But the problem here is that there's a difference between the schema created by the Rust migrations and the schema created by the old Tokenserver. Specifically, the new schema is missing a unique key constraint on (service, node); I'll add a migration to resolve that and a couple of other differences between the schemas. Thanks for the catch! |
1082583
to
708ba57
Compare
I added the necessary migrations and fixed the integration tests |
Also, here is the command I use to run the tests:
|
res.request().clone(), | ||
resp.into_body(), | ||
))) | ||
if res.request().path().starts_with("/1.0/") { |
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.
🤔I wonder if this pattern might creep up enough that it's worth creating a macro or function that encapsulates it?
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.
Do you foresee other types of requests aside from TS ones for which we wouldn't want to render the default Sync 404 response?
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.
No, probably not. Just seemed like a pattern.
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.
To keep things simpler, I'd probably wait to add another macro/function until we have another instance of the pattern, but I'd be happy to add it now if you feel strongly about it!
Thanks @ethowitz, I got local tests passing w/ the new migrations applied and also the e2e tests passing. |
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.
Looks good. You may want to consider supporting a db style URL for the new settings vs individual TOKENSERVER_USERNAME/PASS/DATABASE etc. which is a little tedious (maybe via sqlalchemy's lower level "core" API might not be too much work) depending on how commonly devs/testers would need to set this up.
@pjenvey Done! That change was much easier than I thought it'd be...the APIs are pretty much identical |
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.
Great, thanks!
Description
Adds a suite of integration tests for Tokenserver. These tests can be run remotely against an arbitrary Tokenserver, meaning they can be run against both the old and the new Tokenserver. All of the tests are passing against the old Tokenserver. Some of the tests were adapted from the tests in this file, but most of them are newly-written.
I also made some changes to the Rust code (there were some errors that the newly-added integration tests caught 🥲)
Testing
MockOAuthVerifier
to this file defined like so:Issue(s)
Closes #1048