Skip to content

Update message displayed in the Experimental Debugger banner #1996

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.

Sign up for GitHub

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

Merged
DonJayamanne merged 6 commits into from
Jun 18, 2018
Merged

Update message displayed in the Experimental Debugger banner #1996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4dbde07
Update message displayed
DonJayamanne Jun 18, 2018
cb66e5f
Disable prompting untill next session of VSC
DonJayamanne Jun 18, 2018
f9e415c
Add comment
DonJayamanne Jun 18, 2018
dfad3cc
Changes in logic based on new requirements
DonJayamanne Jun 18, 2018
0694045
Changes in logic based on new requirements
DonJayamanne Jun 18, 2018
4741e4a
Fix grammar
DonJayamanne Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/client/application/diagnostics/checks/envPathVariable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '.
import { DiagnosticScope, IDiagnostic, IDiagnosticHandlerService } from '../types';

const InvalidEnvPathVariableMessage = 'The environment variable \'{0}\' seems to have some paths containing characters (\';\', \'"\', \'%\' or \';;\').' +
' The existence of such characters are known to have caused the {1} extension not load.';
' The existence of such characters are known to have caused the {1} extension to not load.';

export class InvalidEnvironmentPathVariableDiagnostic extends BaseDiagnostic {
constructor(message) {
Expand Down
18 changes: 9 additions & 9 deletions src/client/debugger/banner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export enum PersistentStateKeys {
@injectable()
export class ExperimentalDebuggerBanner implements IExperimentalDebuggerBanner {
private initialized?: boolean;
private disabled?: boolean;
private disabledInCurrentSession?: boolean;
public get enabled(): boolean {
const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
return factory.createGlobalPersistentState<boolean>(PersistentStateKeys.ShowBanner, true).value;
Expand All @@ -39,10 +39,10 @@ export class ExperimentalDebuggerBanner implements IExperimentalDebuggerBanner {
return;
}
const debuggerService = this.serviceContainer.get<IDebugService>(IDebugService);
const disposable = debuggerService.onDidStartDebugSession(e => {
const disposable = debuggerService.onDidStartDebugSession(async e => {
if (e.type === ExperimentalDebuggerType) {
const logger = this.serviceContainer.get<ILogger>(ILogger);
this.onDebugSessionStarted()
await this.onDebugSessionStarted()
.catch(ex => logger.logError('Error in debugger Banner', ex));
}
});
Expand All @@ -51,9 +51,9 @@ export class ExperimentalDebuggerBanner implements IExperimentalDebuggerBanner {
}
public async showBanner(): Promise<void> {
const appShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
const yes = 'Take Survey';
const yes = 'Yes, take survey now';
const no = 'No thanks';
const response = await appShell.showInformationMessage('Can you take 2 minutes to tell us how the Experimental Debugger is working for you?', yes, no);
const response = await appShell.showInformationMessage('Can you please take 2 minutes to tell us how the Experimental Debugger is working for you?', yes, no);
switch (response) {
case yes:
{
Expand All @@ -66,12 +66,13 @@ export class ExperimentalDebuggerBanner implements IExperimentalDebuggerBanner {
break;
}
default: {
return;
// Disable for the current session.
this.disabledInCurrentSession = true;
}
}
}
public async shouldShowBanner(): Promise<boolean> {
if (!this.enabled) {
if (!this.enabled || this.disabledInCurrentSession) {
return false;
}
const [threshold, debuggerCounter] = await Promise.all([this.getDebuggerLaunchThresholdCounter(), this.getGetDebuggerLaunchCounter()]);
Expand All @@ -81,7 +82,6 @@ export class ExperimentalDebuggerBanner implements IExperimentalDebuggerBanner {
public async disable(): Promise<void> {
const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
await factory.createGlobalPersistentState<boolean>(PersistentStateKeys.ShowBanner, false).updateValue(false);
this.disabled = true;
}
public async launchSurvey(): Promise<void> {
const debuggerLaunchCounter = await this.getGetDebuggerLaunchCounter();
Expand Down Expand Up @@ -115,7 +115,7 @@ export class ExperimentalDebuggerBanner implements IExperimentalDebuggerBanner {
return isNaN(num) ? crypto.randomBytes(1).toString('hex').slice(-1) : lastHexValue;
}
private async onDebugSessionStarted(): Promise<void> {
if (this.disabled) {
if (!this.enabled) {
return;
}
await this.incrementDebuggerLaunchCounter();
Expand Down
85 changes: 75 additions & 10 deletions src/test/debugger/banner.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { expect } from 'chai';
import * as typemoq from 'typemoq';
import { DebugSession } from 'vscode';
import { IDebugService } from '../../client/common/application/types';
import { IApplicationShell, IDebugService } from '../../client/common/application/types';
import { IBrowserService, IDisposableRegistry, ILogger, IPersistentState, IPersistentStateFactory } from '../../client/common/types';
import { ExperimentalDebuggerBanner, PersistentStateKeys } from '../../client/debugger/banner';
import { ExperimentalDebuggerType } from '../../client/debugger/Common/constants';
Expand All @@ -22,7 +22,12 @@ suite('Debugging - Banner', () => {
let launchThresholdCounterState: typemoq.IMock<IPersistentState<number | undefined>>;
let showBannerState: typemoq.IMock<IPersistentState<boolean>>;
let debugService: typemoq.IMock<IDebugService>;
let appShell: typemoq.IMock<IApplicationShell>;
let banner: IExperimentalDebuggerBanner;
const message = 'Can you please take 2 minutes to tell us how the Experimental Debugger is working for you?';
const yes = 'Yes, take survey now';
const no = 'No thanks';

setup(() => {
serviceContainer = typemoq.Mock.ofType<IServiceContainer>();
browser = typemoq.Mock.ofType<IBrowserService>();
Expand All @@ -31,6 +36,7 @@ suite('Debugging - Banner', () => {

launchCounterState = typemoq.Mock.ofType<IPersistentState<number>>();
showBannerState = typemoq.Mock.ofType<IPersistentState<boolean>>();
appShell = typemoq.Mock.ofType<IApplicationShell>();
launchThresholdCounterState = typemoq.Mock.ofType<IPersistentState<number | undefined>>();
const factory = typemoq.Mock.ofType<IPersistentStateFactory>();
factory
Expand All @@ -48,6 +54,7 @@ suite('Debugging - Banner', () => {
serviceContainer.setup(s => s.get(typemoq.It.isValue(IDebugService))).returns(() => debugService.object);
serviceContainer.setup(s => s.get(typemoq.It.isValue(ILogger))).returns(() => logger.object);
serviceContainer.setup(s => s.get(typemoq.It.isValue(IDisposableRegistry))).returns(() => []);
serviceContainer.setup(s => s.get(typemoq.It.isValue(IApplicationShell))).returns(() => appShell.object);

banner = new ExperimentalDebuggerBanner(serviceContainer.object);
});
Expand All @@ -63,28 +70,28 @@ suite('Debugging - Banner', () => {
browser.verifyAll();
});
test('Increment Debugger Launch Counter when debug session starts', async () => {
let onDidStartDebugSessionCb: (e: DebugSession) => void;
let onDidStartDebugSessionCb: (e: DebugSession) => Promise<void>;
debugService.setup(d => d.onDidStartDebugSession(typemoq.It.isAny()))
.callback(cb => onDidStartDebugSessionCb = cb)
.verifiable(typemoq.Times.once());

const debuggerLaunchCounter = 1234;
launchCounterState.setup(l => l.value).returns(() => debuggerLaunchCounter)
.verifiable(typemoq.Times.once());
.verifiable(typemoq.Times.atLeastOnce());
launchCounterState.setup(l => l.updateValue(typemoq.It.isValue(debuggerLaunchCounter + 1)))
.verifiable(typemoq.Times.once());
showBannerState.setup(s => s.value).returns(() => true)
.verifiable(typemoq.Times.once());
.verifiable(typemoq.Times.atLeastOnce());

banner.initialize();
onDidStartDebugSessionCb!({ type: ExperimentalDebuggerType } as any);
await onDidStartDebugSessionCb!({ type: ExperimentalDebuggerType } as any);

launchCounterState.verifyAll();
browser.verifyAll();
debugService.verifyAll();
showBannerState.verifyAll();
});
test('Do not Increment Debugger Launch Counter when debug session starts when Banner is disabled', async () => {
test('Do not Increment Debugger Launch Counter when debug session starts and Banner is disabled', async () => {
debugService.setup(d => d.onDidStartDebugSession(typemoq.It.isAny()))
.verifiable(typemoq.Times.never());

Expand All @@ -103,15 +110,15 @@ suite('Debugging - Banner', () => {
debugService.verifyAll();
showBannerState.verifyAll();
});
test('shouldShowBanner returnes false when Banner is disabled', async () => {
test('shouldShowBanner must return false when Banner is disabled', async () => {
showBannerState.setup(s => s.value).returns(() => false)
.verifiable(typemoq.Times.once());

expect(await banner.shouldShowBanner()).to.be.equal(false, 'Incorrect value');

showBannerState.verifyAll();
});
test('shouldShowBanner returnes false when Banner is enabled and debug counter is not same as threshold', async () => {
test('shouldShowBanner must return false when Banner is enabled and debug counter is not same as threshold', async () => {
showBannerState.setup(s => s.value).returns(() => true)
.verifiable(typemoq.Times.once());
launchCounterState.setup(l => l.value).returns(() => 1)
Expand All @@ -125,7 +132,7 @@ suite('Debugging - Banner', () => {
launchCounterState.verifyAll();
launchThresholdCounterState.verifyAll();
});
test('shouldShowBanner returnes true when Banner is enabled and debug counter is same as threshold', async () => {
test('shouldShowBanner must return true when Banner is enabled and debug counter is same as threshold', async () => {
showBannerState.setup(s => s.value).returns(() => true)
.verifiable(typemoq.Times.once());
launchCounterState.setup(l => l.value).returns(() => 10)
Expand All @@ -139,7 +146,65 @@ suite('Debugging - Banner', () => {
launchCounterState.verifyAll();
launchThresholdCounterState.verifyAll();
});
test('Disabling banner should store value of \'false\' in global store', async () => {
test('showBanner must be invoked when shouldShowBanner returns true', async () => {
let onDidStartDebugSessionCb: (e: DebugSession) => Promise<void>;
const currentLaunchCounter = 50;

debugService.setup(d => d.onDidStartDebugSession(typemoq.It.isAny()))
.callback(cb => onDidStartDebugSessionCb = cb)
.verifiable(typemoq.Times.atLeastOnce());
showBannerState.setup(s => s.value).returns(() => true)
.verifiable(typemoq.Times.atLeastOnce());
launchCounterState.setup(l => l.value).returns(() => currentLaunchCounter)
.verifiable(typemoq.Times.atLeastOnce());
launchThresholdCounterState.setup(t => t.value).returns(() => 10)
.verifiable(typemoq.Times.atLeastOnce());
launchCounterState.setup(l => l.updateValue(typemoq.It.isValue(currentLaunchCounter + 1)))
.returns(() => Promise.resolve())
.verifiable(typemoq.Times.atLeastOnce());

appShell.setup(a => a.showInformationMessage(typemoq.It.isValue(message), typemoq.It.isValue(yes), typemoq.It.isValue(no)))
.verifiable(typemoq.Times.once());
banner.initialize();
await onDidStartDebugSessionCb!({ type: ExperimentalDebuggerType } as any);

appShell.verifyAll();
showBannerState.verifyAll();
launchCounterState.verifyAll();
launchThresholdCounterState.verifyAll();
});
test('showBanner must not be invoked the second time after dismissing the message', async () => {
let onDidStartDebugSessionCb: (e: DebugSession) => Promise<void>;
let currentLaunchCounter = 50;

debugService.setup(d => d.onDidStartDebugSession(typemoq.It.isAny()))
.callback(cb => onDidStartDebugSessionCb = cb)
.verifiable(typemoq.Times.atLeastOnce());
showBannerState.setup(s => s.value).returns(() => true)
.verifiable(typemoq.Times.atLeastOnce());
launchCounterState.setup(l => l.value).returns(() => currentLaunchCounter)
.verifiable(typemoq.Times.atLeastOnce());
launchThresholdCounterState.setup(t => t.value).returns(() => 10)
.verifiable(typemoq.Times.atLeastOnce());
launchCounterState.setup(l => l.updateValue(typemoq.It.isAny()))
.callback(() => currentLaunchCounter = currentLaunchCounter + 1);

appShell.setup(a => a.showInformationMessage(typemoq.It.isValue(message), typemoq.It.isValue(yes), typemoq.It.isValue(no)))
.returns(() => Promise.resolve(undefined))
.verifiable(typemoq.Times.once());
banner.initialize();
await onDidStartDebugSessionCb!({ type: ExperimentalDebuggerType } as any);
await onDidStartDebugSessionCb!({ type: ExperimentalDebuggerType } as any);
await onDidStartDebugSessionCb!({ type: ExperimentalDebuggerType } as any);
await onDidStartDebugSessionCb!({ type: ExperimentalDebuggerType } as any);

appShell.verifyAll();
showBannerState.verifyAll();
launchCounterState.verifyAll();
launchThresholdCounterState.verifyAll();
expect(currentLaunchCounter).to.be.equal(54);
});
test('Disabling banner must store value of \'false\' in global store', async () => {
showBannerState.setup(s => s.updateValue(typemoq.It.isValue(false)))
.verifiable(typemoq.Times.once());

Expand Down