-
Notifications
You must be signed in to change notification settings - Fork 7
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 tests for timezone-aware datetimes #98
base: main
Are you sure you want to change the base?
Conversation
Hello @kmshin1397! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-05-03 20:25:01 UTC |
Probably easiest if #100 goes in first |
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. Perhaps also check that when fetching the new user, the created_at column is what is expected?
Hmm not sure why the newly-merged GA workflow isn't running. Maybe it's only triggered for new PRs and new commits to PRs existing before the GA was implemented |
Also not sure what the best practice is here: these tests will technically fail without the changes in cesium-ml/baselayer#187, but this is also designed to test the logic in there. I could commit those baselayer changes here as well (which is what I did when I first made this PR) but that involves pinning the baselayer submodule to a dangling commit and then we'd have to re-pin to the correct commit once the baselayer PR is merged, which also seems bad. For now, just leaving this as is. |
Add tests to accompany my PR on baselayer: cesium-ml/baselayer#187
The baselayer diffs on this PR should be equivalent to pinning to latest baselayer and then adding the changes in that PR on top.