Skip to content

Conversation

@zuoyaofu
Copy link
Contributor

@zuoyaofu zuoyaofu commented Feb 28, 2025

Problem

/review command functionality lacks end-to-end test coverage.

Solution

Add basic E2E tests for file review, project review, and ignore lines features.


  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@zuoyaofu
Copy link
Contributor Author

/runIntegrationTests

@zuoyaofu zuoyaofu marked this pull request as ready for review March 1, 2025 18:15
@zuoyaofu zuoyaofu requested a review from a team as a code owner March 1, 2025 18:15
})

describe('Quick action availability', () => {
console.log('running this test')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can remove this

assert.deepStrictEqual(noFileMessage.type, 'answer')
assert.deepStrictEqual(
noFileMessage.body,
'Sorry, your current active window is not a source code file. Make sure you select a source file as your primary context.'
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to see if you can extract text like this out and share it between your src and test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix

import path from 'path'

function getWorkspaceFolder(): string {
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen a couple teams do the same thing, do we not have a util? Also, we can expect the first workspace folder to load, otherwise the test setup didn't complete successfully

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not able to find the util for this, so I used vscode.workspace.workspaceFolders. You are right that we can expect the workspace to load, I will make a change to reflect that.

const scanResultBody = await waitForReviewResults(tab)

const issues = extractAndValidateIssues(scanResultBody)
assert.deepStrictEqual(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these in some way slightly deterministic? i.e. will we be introducing accidental flake if the backend changes and doesn't emit the right amount of criticals, high, medium, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. Are re-discussing with team, we will change this to only having the critical, rule-based findings that we will not alter. I will make a change for that.

hasExactlyMatchingSecurityDiagnostic(
securityDiagnostics,
'multilanguage-password',
'CWE-798 - Hardcoded credentials',
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise, will these always be deterministic and return the same responses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will make a change to reflect that.

@zuoyaofu
Copy link
Contributor Author

zuoyaofu commented Mar 4, 2025

/runIntegrationTests

@@ -0,0 +1,4 @@
{
"type": "Test",
"description": "add Q Chat /review command test coverage"
Copy link
Contributor

Choose a reason for hiding this comment

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

changelog is not necessary for adding tests.

@justinmk3 justinmk3 merged commit 00f9d3f into aws:master Mar 4, 2025
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants