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

AutoUpdate: tokens and tests #2411

Merged
merged 1 commit into from
Apr 20, 2018
Merged

Conversation

HebaruSan
Copy link
Member

Problem

#2344 fixed a JSON parsing problem that broke the auto updater for v1.24.0. This problem could and should have been caught by existing tests, but they weren't running on Travis, and when they did run, they failed due to TLS errors in their network requests (also fixed in #2344) rather than the real problem affecting the client.

#2263 added support for GitHub API auth tokens, but they are not currently used by the auto updater.

Cause

AutoUpdate has tests, but they're in the FlakyNetwork category and therefore don't run on Travis. This makes it harder to notice that you've broken the auto updater. When these tests do run, they rely on the network to provide input data, so failures cause by network problems can mask problems with other parts of the logic such as the JSON parsing.

AutoUpdate is not structured to be testable. It has many different responsibilities (checking for updates, parsing update metadata, downloading new client, running new client), which can't be separated out for isolated testing.

Changes

Now AutoUpdate is refactored to better isolate the network-dependent part of what it does, and a new test is added that exercises the parsing code without relying on the network at all. This test would have caught the problem that #2344 fixed, and now it will run on Travis every time.

One of the old tests is removed because it only fetched network data using functions that were removed in the refactoring (for a repo with no releases), but the rest are still there, so you can still use them to perform combined network-and-parsing tests if needed.

Net.DownloadText is updated to support auth tokens, and AutoUpdate uses it to get release info. So the auto updater now supports auth tokens.

@HebaruSan HebaruSan added Pull request AutoUpdate Issues affecting the automatic updating Tests Issues affecting the internal tests Network Issues affecting internet connections of CKAN and removed Network Issues affecting internet connections of CKAN labels Apr 15, 2018
@politas politas merged commit 8e89491 into KSP-CKAN:master Apr 20, 2018
politas added a commit that referenced this pull request Apr 20, 2018
@HebaruSan HebaruSan deleted the fix/test-autoupdate branch April 20, 2018 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoUpdate Issues affecting the automatic updating Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants