Skip to content

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

@kmshin1397 kmshin1397 requested review from acrellin and stefanv May 3, 2021 16:24
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.

3 participants