Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Feature",
"description": "/review: Code issues can be grouped by file location or severity"
}
13 changes: 12 additions & 1 deletion packages/amazonq/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -370,10 +370,15 @@
"when": "view == aws.AmazonQChatView || view == aws.amazonq.AmazonCommonAuth",
"group": "y_toolkitMeta@2"
},
{
"command": "aws.amazonq.codescan.showGroupingStrategy",
"when": "view == aws.amazonq.SecurityIssuesTree",
"group": "navigation@1"
},
{
"command": "aws.amazonq.security.showFilters",
"when": "view == aws.amazonq.SecurityIssuesTree",
"group": "navigation"
"group": "navigation@2"
}
],
"view/item/context": [
Expand Down Expand Up @@ -729,6 +734,12 @@
{
"command": "aws.amazonq.security.showFilters",
"title": "%AWS.command.amazonq.filterIssues%",
"icon": "$(filter)",
"enablement": "view == aws.amazonq.SecurityIssuesTree"
},
{
"command": "aws.amazonq.codescan.showGroupingStrategy",
"title": "%AWS.command.amazonq.groupIssues%",
"icon": "$(list-filter)",
"enablement": "view == aws.amazonq.SecurityIssuesTree"
},
Expand Down
116 changes: 115 additions & 1 deletion packages/amazonq/test/unit/codewhisperer/models/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@
*/
import assert from 'assert'
import sinon from 'sinon'
import { SecurityIssueFilters, SecurityTreeViewFilterState } from 'aws-core-vscode/codewhisperer'
import {
CodeIssueGroupingStrategy,
CodeIssueGroupingStrategyState,
SecurityIssueFilters,
SecurityTreeViewFilterState,
} from 'aws-core-vscode/codewhisperer'
import { globals } from 'aws-core-vscode/shared'

describe('model', function () {
Expand Down Expand Up @@ -70,4 +75,113 @@ describe('model', function () {
assert.deepStrictEqual(hiddenSeverities, ['High', 'Low'])
})
})

describe('CodeIssueGroupingStrategyState', function () {
let sandbox: sinon.SinonSandbox
let state: CodeIssueGroupingStrategyState

beforeEach(function () {
sandbox = sinon.createSandbox()
// Reset the singleton instance before each test
// @ts-ignore - accessing private static for testing
CodeIssueGroupingStrategyState['#instance'] = undefined
state = CodeIssueGroupingStrategyState.instance
})

afterEach(function () {
sandbox.restore()
})

describe('instance', function () {
it('should return the same instance when called multiple times', function () {
const instance1 = CodeIssueGroupingStrategyState.instance
const instance2 = CodeIssueGroupingStrategyState.instance
assert.strictEqual(instance1, instance2)
})
})

describe('getState', function () {
it('should return fallback when no state is stored', function () {
const tryGetStub = sandbox.stub(globals.globalState, 'tryGet').returns(undefined)
const result = state.getState()

sinon.assert.calledWith(tryGetStub, 'aws.amazonq.codescan.groupingStrategy', String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these kinds of tests are very fragile. can you assert the actual state instead of using stubs/mocks?

assert.equal(result, CodeIssueGroupingStrategy.Severity)
})

it('should return stored state when valid', function () {
const validStrategy = CodeIssueGroupingStrategy.Severity
sandbox.stub(globals.globalState, 'tryGet').returns(validStrategy)

const result = state.getState()

assert.equal(result, validStrategy)
})

it('should return fallback when stored state is invalid', function () {
const invalidStrategy = 'invalid'
sandbox.stub(globals.globalState, 'tryGet').returns(invalidStrategy)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just set the globalState? the tests have a fake globalState, it is safe to set it.

sinon and stubs aren't needed.


const result = state.getState()

assert.equal(result, CodeIssueGroupingStrategy.Severity)
})
})

describe('setState', function () {
it('should update state and fire change event for valid strategy', async function () {
const validStrategy = CodeIssueGroupingStrategy.FileLocation
const updateStub = sandbox.stub(globals.globalState, 'update').resolves()

// Create a spy to watch for event emissions
const eventSpy = sandbox.spy()
state.onDidChangeState(eventSpy)

await state.setState(validStrategy)

sinon.assert.calledWith(updateStub, 'aws.amazonq.codescan.groupingStrategy', validStrategy)
sinon.assert.calledWith(eventSpy, validStrategy)
})

it('should use fallback and fire change event for invalid strategy', async function () {
const invalidStrategy = 'invalid'
const updateStub = sandbox.stub(globals.globalState, 'update').resolves()

// Create a spy to watch for event emissions
const eventSpy = sandbox.spy()
state.onDidChangeState(eventSpy)

await state.setState(invalidStrategy)

sinon.assert.calledWith(
updateStub,
'aws.amazonq.codescan.groupingStrategy',
CodeIssueGroupingStrategy.Severity
)
sinon.assert.calledWith(eventSpy, CodeIssueGroupingStrategy.Severity)
})
})

describe('reset', function () {
it('should set state to fallback value', async function () {
const setStateStub = sandbox.stub(state, 'setState').resolves()

await state.reset()

sinon.assert.calledWith(setStateStub, CodeIssueGroupingStrategy.Severity)
})
})

describe('onDidChangeState', function () {
it('should allow subscribing to state changes', async function () {
const listener = sandbox.spy()
const disposable = state.onDidChangeState(listener)

await state.setState(CodeIssueGroupingStrategy.Severity)

sinon.assert.calledWith(listener, CodeIssueGroupingStrategy.Severity)
disposable.dispose()
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,24 @@ import {
SecurityTreeViewFilterState,
SecurityIssueProvider,
SeverityItem,
CodeIssueGroupingStrategyState,
CodeIssueGroupingStrategy,
} from 'aws-core-vscode/codewhisperer'
import { createCodeScanIssue } from 'aws-core-vscode/test'
import assert from 'assert'
import sinon from 'sinon'
import path from 'path'

describe('SecurityIssueTreeViewProvider', function () {
let securityIssueProvider: SecurityIssueProvider
let securityIssueTreeViewProvider: SecurityIssueTreeViewProvider

beforeEach(function () {
securityIssueProvider = SecurityIssueProvider.instance
SecurityIssueProvider.instance.issues = [
{ filePath: 'file/path/a', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/b', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/c', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/d', issues: [createCodeScanIssue(), createCodeScanIssue()] },
]
securityIssueTreeViewProvider = new SecurityIssueTreeViewProvider()
})

Expand All @@ -44,13 +51,6 @@ describe('SecurityIssueTreeViewProvider', function () {

describe('getChildren', function () {
it('should return sorted list of severities if element is undefined', function () {
securityIssueProvider.issues = [
{ filePath: 'file/path/c', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/d', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/a', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/b', issues: [createCodeScanIssue(), createCodeScanIssue()] },
]

const element = undefined
const result = securityIssueTreeViewProvider.getChildren(element) as SeverityItem[]
assert.strictEqual(result.length, 5)
Expand Down Expand Up @@ -102,5 +102,55 @@ describe('SecurityIssueTreeViewProvider', function () {
const result = securityIssueTreeViewProvider.getChildren(element) as IssueItem[]
assert.strictEqual(result.length, 0)
})

it('should return severity-grouped items when grouping strategy is Severity', function () {
sinon.stub(CodeIssueGroupingStrategyState.instance, 'getState').returns(CodeIssueGroupingStrategy.Severity)

const severityItems = securityIssueTreeViewProvider.getChildren() as SeverityItem[]
for (const [index, [severity, expectedIssueCount]] of [
['Critical', 0],
['High', 8],
['Medium', 0],
['Low', 0],
['Info', 0],
].entries()) {
const currentSeverityItem = severityItems[index]
assert.strictEqual(currentSeverityItem.label, severity)
assert.strictEqual(currentSeverityItem.issues.length, expectedIssueCount)

const issueItems = securityIssueTreeViewProvider.getChildren(currentSeverityItem) as IssueItem[]
assert.ok(issueItems.every((item) => item.iconPath === undefined))
assert.ok(
issueItems.every((item) => item.description?.toString().startsWith(path.basename(item.filePath)))
)
}
})

it('should return file-grouped items when grouping strategy is FileLocation', function () {
sinon
.stub(CodeIssueGroupingStrategyState.instance, 'getState')
.returns(CodeIssueGroupingStrategy.FileLocation)

const result = securityIssueTreeViewProvider.getChildren() as FileItem[]
for (const [index, [fileName, expectedIssueCount]] of [
['a', 2],
['b', 2],
['c', 2],
['d', 2],
].entries()) {
const currentFileItem = result[index]
assert.strictEqual(currentFileItem.label, fileName)
assert.strictEqual(currentFileItem.issues.length, expectedIssueCount)
assert.strictEqual(currentFileItem.description, 'file/path')

const issueItems = securityIssueTreeViewProvider.getChildren(currentFileItem) as IssueItem[]
assert.ok(
issueItems.every((item) =>
item.iconPath?.toString().includes(`${item.issue.severity.toLowerCase()}.svg`)
)
)
assert.ok(issueItems.every((item) => item.description?.toString().startsWith('[Ln ')))
}
})
})
})
35 changes: 35 additions & 0 deletions packages/amazonq/test/unit/codewhisperer/ui/prompters.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import { createQuickPickPrompterTester, QuickPickPrompterTester } from 'aws-core-vscode/test'
import {
CodeIssueGroupingStrategy,
CodeIssueGroupingStrategyState,
createCodeIssueGroupingStrategyPrompter,
} from 'aws-core-vscode/codewhisperer'
import sinon from 'sinon'

const severity = { label: 'Severity', data: CodeIssueGroupingStrategy.Severity }
const fileLocation = { label: 'File Location', data: CodeIssueGroupingStrategy.FileLocation }

describe('createCodeIssueGroupingStrategyPrompter', function () {
let tester: QuickPickPrompterTester<CodeIssueGroupingStrategy>

beforeEach(function () {
tester = createQuickPickPrompterTester(createCodeIssueGroupingStrategyPrompter())
})

it('should list grouping strategies', function () {
tester.assertItems([severity, fileLocation])
})

it('should update state on selection', async function () {
const spy = sinon.spy(CodeIssueGroupingStrategyState.instance, 'setState')
Copy link
Contributor

@justinmk3 justinmk3 Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use "singletons", which presumably are intended to make modules testable. Do we need to stub the global singleton, or can we create a new singleton "object" for testing?

If not, then there is zero reason to have singletons at all, we might as well use plain old modules.


tester.selectItems(fileLocation)
tester.assertSelectedItems(fileLocation)

spy.calledWith(CodeIssueGroupingStrategy.FileLocation)
})
})
5 changes: 5 additions & 0 deletions packages/core/package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@
"AWS.command.amazonq.acceptFix": "Accept Fix",
"AWS.command.amazonq.regenerateFix": "Regenerate Fix",
"AWS.command.amazonq.filterIssues": "Filter Issues",
"AWS.command.amazonq.groupIssues": "Group Issues",
"AWS.command.deploySamApplication": "Deploy SAM Application",
"AWS.command.aboutToolkit": "About",
"AWS.command.downloadLambda": "Download...",
Expand Down Expand Up @@ -309,6 +310,10 @@
"AWS.amazonq.scans.projectScanInProgress": "Workspace review is in progress...",
"AWS.amazonq.scans.fileScanInProgress": "File review is in progress...",
"AWS.amazonq.scans.noGitRepo": "Your workspace is not in a git repository. I'll review your project files for security issues, and your in-flight changes for code quality issues.",
"AWS.amazonq.scans.severity": "Severity",
"AWS.amazonq.scans.fileLocation": "File Location",
"AWS.amazonq.scans.groupIssues": "Group Issues",
"AWS.amazonq.scans.groupIssues.placeholder": "Select how to group code issues",
"AWS.amazonq.featureDev.error.conversationIdNotFoundError": "Conversation id must exist before starting code generation",
"AWS.amazonq.featureDev.error.contentLengthError": "The folder you selected is too large for me to use as context. Please choose a smaller folder to work on. For more information on quotas, see the <a href=\"https://docs.aws.amazon.com/amazonq/latest/qdeveloper-ug/software-dev.html#quotas\" target=\"_blank\">Amazon Q Developer documentation.</a>",
"AWS.amazonq.featureDev.error.illegalStateTransition": "Illegal transition between states, restart the conversation",
Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/codewhisperer/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
SecurityTreeViewFilterState,
AggregatedCodeScanIssue,
CodeScanIssue,
CodeIssueGroupingStrategyState,
} from './models/model'
import { invokeRecommendation } from './commands/invokeRecommendation'
import { acceptSuggestion } from './commands/onInlineAcceptance'
Expand Down Expand Up @@ -60,6 +61,7 @@ import {
ignoreAllIssues,
focusIssue,
showExploreAgentsView,
showCodeIssueGroupingQuickPick,
} from './commands/basicCommands'
import { sleep } from '../shared/utilities/timeoutUtils'
import { ReferenceLogViewProvider } from './service/referenceLogViewProvider'
Expand Down Expand Up @@ -288,6 +290,8 @@ export async function activate(context: ExtContext): Promise<void> {
listCodeWhispererCommands.register(),
// quick pick with security issues tree filters
showSecurityIssueFilters.register(),
// quick pick code issue grouping strategy
showCodeIssueGroupingQuickPick.register(),
// reset security issue filters
clearFilters.register(),
// handle security issues tree item clicked
Expand All @@ -296,6 +300,10 @@ export async function activate(context: ExtContext): Promise<void> {
SecurityTreeViewFilterState.instance.onDidChangeState((e) => {
SecurityIssueTreeViewProvider.instance.refresh()
}),
// refresh treeview when grouping strategy changes
CodeIssueGroupingStrategyState.instance.onDidChangeState((e) => {
SecurityIssueTreeViewProvider.instance.refresh()
}),
// show a no match state
SecurityIssueTreeViewProvider.instance.onDidChangeTreeData((e) => {
const noMatches =
Expand Down
9 changes: 9 additions & 0 deletions packages/core/src/codewhisperer/commands/basicCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import { startCodeFixGeneration } from './startCodeFixGeneration'
import { DefaultAmazonQAppInitContext } from '../../amazonq/apps/initContext'
import path from 'path'
import { parsePatch } from 'diff'
import { createCodeIssueGroupingStrategyPrompter } from '../ui/prompters'

const MessageTimeOut = 5_000

Expand Down Expand Up @@ -884,6 +885,14 @@ export const showSecurityIssueFilters = Commands.declare({ id: 'aws.amazonq.secu
}
})

export const showCodeIssueGroupingQuickPick = Commands.declare(
{ id: 'aws.amazonq.codescan.showGroupingStrategy' },
() => async () => {
const prompter = createCodeIssueGroupingStrategyPrompter()
await prompter.prompt()
}
)

export const focusIssue = Commands.declare(
{ id: 'aws.amazonq.security.focusIssue' },
() => async (issue: CodeScanIssue, filePath: string) => {
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/codewhisperer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,3 +101,4 @@ export * as CodeWhispererConstants from '../codewhisperer/models/constants'
export { getSelectedCustomization } from './util/customizationUtil'
export { Container } from './service/serviceContainer'
export * from './util/gitUtil'
export * from './ui/prompters'
Loading
Loading