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 the live tracking API endpoints #2305

Merged
merged 1 commit into from
Dec 21, 2020

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Dec 20, 2020

Hey @Turbo87,

in order to prepare for the changes related to #2277 I have first added unit tests to test the current behavior.

Could you please review ?

Thanks

@vicb
Copy link
Contributor Author

vicb commented Dec 20, 2020

I fixed the CI error by sticking to ubuntu 18.04 for now.

What about merging that and create a separate issue to update to ubuntu latest ?

with patch("skylines.model.tracking.datetime") as datetime_mock:
datetime_mock.utcnow.return_value = utcnow

res = client.get("/tracking/latest.json")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at this I was wondering if any of our code was actually using this API endpoint at all and I couldn't find anything. might be worth it to just remove it instead of keeping it alive 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Turbo87 I am not exactly sure what you mean here ?

If by "my code" you mean flyxc then I am not using it as of now, that's right.
However I think @indyflyersoft is using it.

Did I get your question correctly ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what I meant was that the endpoint is not used in the skylines frontend code itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOps "our code" I read too fast. Thanks for the clarification.

Might be worth checking the server logs to see how often it is used.

I think it's handy to have it.
For example flyxc could request the pilot's positions (/live/<user_ids>) only if they are listed here.
And on the other hand there is no real reason to remove this.

Just my 2 cents. I don't have strong opinions on whether keeping/dropping this one.

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thanks

@Turbo87 Turbo87 merged commit 5e9a31b into skylines-project:master Dec 21, 2020
@vicb
Copy link
Contributor Author

vicb commented Dec 21, 2020

Thanks for merging the code @Turbo87.

I'll submit my updates for #2277 in the coming days.

@vicb vicb deleted the tracking_tests branch December 21, 2020 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants