From ac94e5a02755404f67f0f98a69eb7fdc781e5b88 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Date: Mon, 8 Jul 2019 13:54:49 -0700 Subject: [PATCH 1/3] 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 --- src/client/common/crypto.ts | 8 +++---- src/client/common/experiments.ts | 2 +- src/client/common/types.ts | 4 +--- src/test/common/crypto.unit.test.ts | 30 ++++++++++++++++++++++-- src/test/common/experiments.unit.test.ts | 6 ++--- 5 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/client/common/crypto.ts b/src/client/common/crypto.ts index c25ea6cdc089..1d599737d46e 100644 --- a/src/client/common/crypto.ts +++ b/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(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(data: string, hashFormat: E): IHashFormat[E] { + const hash = createHash('sha512').update(data).digest('hex'); + return hashFormat === 'number' ? parseInt(hash, 16) : hash as any; } } diff --git a/src/client/common/experiments.ts b/src/client/common/experiments.ts index 9fcc87c5525e..27fefd8bc67d 100644 --- a/src/client/common/experiments.ts +++ b/src/client/common/experiments.ts @@ -148,7 +148,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; } diff --git a/src/client/common/types.ts b/src/client/common/types.ts index 6c0c390bc7ee..f55fcea6e35e 100644 --- a/src/client/common/types.ts +++ b/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'; @@ -485,10 +484,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(data: string, encoding: HexBase64Latin1Encoding, hashFormat: E): IHashFormat[E]; + createHash(data: string, hashFormat: E): IHashFormat[E]; } export const IAsyncDisposableRegistry = Symbol('IAsyncDisposableRegistry'); diff --git a/src/test/common/crypto.unit.test.ts b/src/test/common/crypto.unit.test.ts index a7274f3900af..6dfef4737eef 100644 --- a/src/test/common/crypto.unit.test.ts +++ b/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'); + }); }); diff --git a/src/test/common/experiments.unit.test.ts b/src/test/common/experiments.unit.test.ts index 870f47f665fb..ef38ffbc8935 100644 --- a/src/test/common/experiments.unit.test.ts +++ b/src/test/common/experiments.unit.test.ts @@ -599,10 +599,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'); } }); @@ -636,7 +636,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); From d63c40c45b7c206bab8da2b2884ec8746d9fe881 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Date: Tue, 9 Jul 2019 14:20:53 -0700 Subject: [PATCH 2/3] Release cherry pick (#6515) * Enable experiment for always displaying the test explorer (#6330) * Add logging --- src/client/common/crypto.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/client/common/crypto.ts b/src/client/common/crypto.ts index 1d599737d46e..c0115ddf8b9f 100644 --- a/src/client/common/crypto.ts +++ b/src/client/common/crypto.ts @@ -6,6 +6,7 @@ import { createHash } from 'crypto'; import { injectable } from 'inversify'; +import { traceError } from './logger'; import { ICryptoUtils, IHashFormat } from './types'; /** @@ -15,6 +16,13 @@ import { ICryptoUtils, IHashFormat } from './types'; export class CryptoUtils implements ICryptoUtils { public createHash(data: string, hashFormat: E): IHashFormat[E] { const hash = createHash('sha512').update(data).digest('hex'); - return hashFormat === 'number' ? parseInt(hash, 16) : hash as any; + if (hashFormat === 'number') { + const result = parseInt(hash, 16); + if (isNaN(result)) { + traceError(`Number hash for data '${data}' is NaN`); + } + return result as any; + } + return hash as any; } } From c3047f7102a799bdc25c7552987919da52d59d7d Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 9 Jul 2019 14:51:39 -0700 Subject: [PATCH 3/3] Point release - Updated package.json and CHANGELOG (#6516) --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8943aa947ac2..ff19cd03a647 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,12 @@ And finally thanks to the [Python](https://www.python.org/) development team and community for creating a fantastic programming language and community to be a part of! +## 2019.6.1 (9 July 2019) + +### Fixes + +1. Fixes to A/B testing. + ([#6400](https://github.com/microsoft/vscode-python/issues/6400)) ## 2019.6.0 (25 June 2019)