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 tests for timezone-aware datetimes #98

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kmshin1397
Copy link
Collaborator

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.

@pep8speaks
Copy link

pep8speaks commented Mar 24, 2021

Hello @kmshin1397! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 50:80: E501 line too long (85 > 79 characters)

Comment last updated at 2021-05-03 20:25:01 UTC

@kmshin1397
Copy link
Collaborator Author

Probably easiest if #100 goes in first

Copy link
Member

@acrellin acrellin left a 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?

@kmshin1397
Copy link
Collaborator Author

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

@kmshin1397
Copy link
Collaborator Author

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.

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.

None yet

3 participants