Fix experiments hash parsing (#6492)

* Simplify & fix crypto.createHash
* Add tests
* Fix python test in news folder (#6398)
* Fix broken python tests in news
* Revert changes

Author: kimadeline

news/test_announce.py

@@ -187,7 +187,8 @@ def fake_git_rm(path):
 
 def test_complete_news():
     version = "2019.3.0"
-    date = datetime.date.today().strftime("%d %B %Y")
+    # Remove leading `0`.
+    date = datetime.date.today().strftime("%d %B %Y").lstrip("0")
     news = ann.complete_news(version, NEW_NEWS, f"{TITLE}\n\n\n{OLD_NEWS}")
     expected = f"{TITLE}\n\n## {version} ({date})\n\n{NEW_NEWS.strip()}\n\n\n{OLD_NEWS.strip()}"
     assert news == expected

src/client/common/crypto.ts

@@ -4,7 +4,7 @@
 
 // tslint:disable: no-any
 
-import { createHash, HexBase64Latin1Encoding } from 'crypto';
+import { createHash } from 'crypto';
 import { injectable } from 'inversify';
 import { ICryptoUtils, IHashFormat } from './types';
 
@@ -13,8 +13,8 @@ import { ICryptoUtils, IHashFormat } from './types';
  */
 @injectable()
 export class CryptoUtils implements ICryptoUtils {
-    public createHash<E extends keyof IHashFormat>(data: string, encoding: HexBase64Latin1Encoding, hashFormat: E): IHashFormat[E] {
-        const hash = createHash('sha512').update(data).digest(encoding);
-        return hashFormat === 'number' ? parseInt(hash, undefined) : hash as any;
+    public createHash<E extends keyof IHashFormat>(data: string, hashFormat: E): IHashFormat[E] {
+        const hash = createHash('sha512').update(data).digest('hex');
+        return hashFormat === 'number' ? parseInt(hash, 16) : hash as any;
     }
 }

src/client/common/experiments.ts

@@ -150,7 +150,7 @@ export class ExperimentsManager implements IExperimentsManager {
         if (typeof (this.appEnvironment.machineId) !== 'string') {
             throw new Error('Machine ID should be a string');
         }
-        const hash = this.crypto.createHash(`${this.appEnvironment.machineId}+${salt}`, 'hex', 'number');
+        const hash = this.crypto.createHash(`${this.appEnvironment.machineId}+${salt}`, 'number');
         return hash % 100 >= min && hash % 100 < max;
     }
 

src/client/common/types.ts

@@ -2,7 +2,6 @@
 // Licensed under the MIT License.
 'use strict';
 
-import { HexBase64Latin1Encoding } from 'crypto';
 import { Socket } from 'net';
 import { Request as RequestResult } from 'request';
 import { ConfigurationTarget, DiagnosticSeverity, Disposable, DocumentSymbolProvider, Event, Extension, ExtensionContext, OutputChannel, Uri, WorkspaceEdit } from 'vscode';
@@ -439,10 +438,9 @@ export interface ICryptoUtils {
      * Creates hash using the data and encoding specified
      * @returns hash as number, or string
      * @param data The string to hash
-     * @param encoding Data encoding to use
      * @param hashFormat Return format of the hash, number or string
      */
-    createHash<E extends keyof IHashFormat>(data: string, encoding: HexBase64Latin1Encoding, hashFormat: E): IHashFormat[E];
+    createHash<E extends keyof IHashFormat>(data: string, hashFormat: E): IHashFormat[E];
 }
 
 export const IAsyncDisposableRegistry = Symbol('IAsyncDisposableRegistry');

src/test/common/crypto.unit.test.ts

@@ -12,11 +12,37 @@ suite('Crypto Utils', async () => {
         crypto = new CryptoUtils();
     });
     test('If hashFormat equals `number`, method createHash() returns a number', async () => {
-        const hash = crypto.createHash('blabla', 'hex', 'number');
+        const hash = crypto.createHash('blabla', 'number');
         assert.typeOf(hash, 'number', 'Type should be a number');
     });
     test('If hashFormat equals `string`, method createHash() returns a string', async () => {
-        const hash = crypto.createHash('blabla', 'hex', 'string');
+        const hash = crypto.createHash('blabla', 'string');
         assert.typeOf(hash, 'string', 'Type should be a string');
     });
+    test('If hashFormat equals `number`, the hash should not be NaN', async () => {
+        let hash = crypto.createHash('test', 'number');
+        assert.isNotNaN(hash, 'Number hash should not be NaN');
+        hash = crypto.createHash('hash', 'number');
+        assert.isNotNaN(hash, 'Number hash should not be NaN');
+        hash = crypto.createHash('HASH1', 'number');
+        assert.isNotNaN(hash, 'Number hash should not be NaN');
+    });
+    test('If hashFormat equals `string`, the hash should not be undefined', async () => {
+        let hash = crypto.createHash('test', 'string');
+        assert.isDefined(hash, 'String hash should not be undefined');
+        hash = crypto.createHash('hash', 'string');
+        assert.isDefined(hash, 'String hash should not be undefined');
+        hash = crypto.createHash('HASH1', 'string');
+        assert.isDefined(hash, 'String hash should not be undefined');
+    });
+    test('If hashFormat equals `number`, hashes with different data should return different number hashes', async () => {
+        const hash1 = crypto.createHash('hash1', 'number');
+        const hash2 = crypto.createHash('hash2', 'number');
+        assert.notEqual(hash1, hash2, 'Hashes should be different numbers');
+    });
+    test('If hashFormat equals `string`, hashes with different data should return different string hashes', async () => {
+        const hash1 = crypto.createHash('hash1', 'string');
+        const hash2 = crypto.createHash('hash2', 'string');
+        assert.notEqual(hash1, hash2, 'Hashes should be different strings');
+    });
 });

src/test/common/experiments.unit.test.ts

@@ -588,10 +588,10 @@ suite('A/B experiments', () => {
                     expect(() => expManager.isUserInRange(79, 94, 'salt')).to.throw();
                 } else if (testParams.error) {
                     const error = new Error('Kaboom');
-                    when(crypto.createHash(anything(), 'hex', 'number')).thenThrow(error);
+                    when(crypto.createHash(anything(), 'number')).thenThrow(error);
                     expect(() => expManager.isUserInRange(79, 94, 'salt')).to.throw(error);
                 } else {
-                    when(crypto.createHash(anything(), 'hex', 'number')).thenReturn(testParams.hash);
+                    when(crypto.createHash(anything(), 'number')).thenReturn(testParams.hash);
                     expect(expManager.isUserInRange(79, 94, 'salt')).to.equal(testParams.expectedResult, 'Incorrectly identified');
                 }
             });
@@ -625,7 +625,7 @@ suite('A/B experiments', () => {
                 .returns(() => testParams.experimentStorageValue);
             when(appEnvironment.machineId).thenReturn('101');
             if (testParams.hash) {
-                when(crypto.createHash(anything(), 'hex', 'number')).thenReturn(testParams.hash);
+                when(crypto.createHash(anything(), 'number')).thenReturn(testParams.hash);
             }
             expManager.populateUserExperiments();
             assert.deepEqual(expManager.userExperiments, testParams.expectedResult);