Update to latest Language Server #2234
Conversation
- change the download version to latest build - change the name of the executable bit of the LS
- Remove hardcoded strings causing issues - Expose information from downloader/platform - Update tests for explicit OS types
Codecov Report
@@ Coverage Diff @@
## master #2234 +/- ##
==========================================
- Coverage 79.84% 79.82% -0.03%
==========================================
Files 310 310
Lines 14359 14367 +8
Branches 2547 2549 +2
==========================================
+ Hits 11465 11468 +3
- Misses 2882 2887 +5
Partials 12 12
Continue to review full report at Codecov.
|
|
VSTS broke - macOS agents have 3.7 installed instead of 3.6 now. |
- make it allow 2, or 3, version segments - handles old M.m.r or new Y.r version patterns
src/client/activation/downloader.ts
Outdated
| const downloadBaseFileName = 'Python-Language-Server'; | ||
| const downloadVersion = '0.1.0'; | ||
| const downloadFileExtension = '.nupkg'; | ||
| export const downloadUriPrefix = 'https://pvsc.blob.core.windows.net/python-language-server'; |
There was a problem hiding this comment.
Please do not export. This is not a valid way of testing.
E.g. if you have a typo or wrong version in here, then when testing you're not testing against expected values.
When writing the test, we should be testing the results against some expected values. Not against the result itself.
I.e. you need to confirm the right version and file is being downloaded.
However if we use the same constant for the test and the code, then we'll never catch any bugs/typos. It'll always pass, i.e. the test does not serve a purpose.
There was a problem hiding this comment.
Sure, but I'm also just as likely to copy and paste between the code and the test, so I don't see this as really protecting you from anything other than adding overhead in tests when these values change.
What Derek should (and soon will) be doing is exporting the object containing the final URLs.
There was a problem hiding this comment.
just as likely to copy and paste between the code and the test
That's always likely to happen in any test.
from anything other than adding overhead in tests when these values change.
But then what's the value of the test.
Isn't this similar to testing a function sum that adds two numbers, and then using the same exported code to validate the sum of a + b. I.e. we should validate the logic by performing the same operation in the test, not by re-importing the same code.
Or a better example is calculation of a hash, we wouldn't re-import the same library to validate the hash. Instead we'd either use some pre-calculated hash in the test.
| import * as assert from 'assert'; | ||
| import * as TypeMoq from 'typemoq'; | ||
| import { LanguageServerDownloader } from '../../client/activation/downloader'; | ||
| import { downloadBaseFileName, downloadFileExtension, downloadUriPrefix, downloadVersion, LanguageServerDownloader } from '../../client/activation/downloader'; |
There was a problem hiding this comment.
Please do not import, leave the values back the way it was. Else its not a valid test.
I.e. if we fail to change version or file extension, tests will never catch it.
- keep it simple, keeler.
* Update to latest Language Server - change the download version to latest build - change the name of the executable bit of the LS * Fix unit tests - Remove hardcoded strings causing issues - Expose information from downloader/platform - Update tests for explicit OS types * Update PIP version regex - make it allow 2, or 3, version segments - handles old M.m.r or new Y.r version patterns * Remove hard-coded strings /and/ formats from tests. * Correction to the PIP_VERSION_REGEX - keep it simple, keeler.
3 participants
Fixes #2233
Includes a news entry file (remember to thank yourself!)"1.2.3", not"^1.2.3")package-lock.jsonhas been regenerated if dependencies have changed