-
Notifications
You must be signed in to change notification settings - Fork 247
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.
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
fix(Amazon Q): fix for test generation target file missing from payload. #5261
Changes from 8 commits
526d8c9
e182ec6
d943f8b
346cee3
556f3f1
a394c85
9276a46
e7544f0
f19f44a
7365162
37dc441
4662cca
62f896d
89545c2
1a8c609
a48f031
7d2979c
86efb24
812c790
12fc617
fd92af6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"type" : "bugfix", | ||
"description" : "Amazon Q /test: Fix for test generation payload creation to not filter out target file." | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
|
||
import com.intellij.openapi.vfs.VirtualFile | ||
import com.intellij.testFramework.RuleChain | ||
import org.junit.Assert.assertEquals | ||
import org.junit.Assert.assertFalse | ||
import org.junit.Assert.assertTrue | ||
import org.junit.Before | ||
|
@@ -75,6 +76,7 @@ class FeatureDevSessionContextTest : FeatureDevTestBase(HeavyJavaCodeInsightTest | |
@Test | ||
fun testZipProject() { | ||
addFilesToProjectModule( | ||
".gitignore", | ||
".gradle/cached.jar", | ||
"src/MyClass.java", | ||
"gradlew", | ||
|
@@ -83,6 +85,19 @@ class FeatureDevSessionContextTest : FeatureDevTestBase(HeavyJavaCodeInsightTest | |
"settings.gradle", | ||
"build.gradle", | ||
"gradle/wrapper/gradle-wrapper.properties", | ||
"builder/GetTestBuilder.java", //check for false positives | ||
".aws-sam/build/function1", | ||
".gem/specs.rb", | ||
"archive.zip", | ||
"output.bin", | ||
"images/logo.png", | ||
"assets/header.jpg", | ||
"icons/menu.svg", | ||
"license.txt", | ||
"License.md", | ||
"node_modules/express", | ||
"build/outputs", | ||
"dist/bundle.js" | ||
) | ||
|
||
val zipResult = featureDevSessionContext.getProjectZip() | ||
|
@@ -102,11 +117,73 @@ class FeatureDevSessionContextTest : FeatureDevTestBase(HeavyJavaCodeInsightTest | |
"gradlew", | ||
"gradlew.bat", | ||
"README.md", | ||
"settings.gradle", | ||
"build.gradle", | ||
"gradle/wrapper/gradle-wrapper.properties", | ||
"builder/GetTestBuilder.java" | ||
) | ||
|
||
assertTrue(zippedFiles == expectedFiles) | ||
} | ||
|
||
@Test | ||
fun `test basic pattern conversion`() { | ||
val input = "*.txt" | ||
val expected = "(?:^|.*/?)[^/]*[^/]\\.txt(?:/.*)?\$" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do I validate if this is correct? This appears to be a snapshot test of what is built versus a functional test that demonstrates that what is an example file that would've been dropped is no longer dropped There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can test for pattern matching as well. That's why i included more files in the project zip test to validate it. Including false positives. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point is that these tests do little to validate behaviors and protect against regressions. If we need to update the logic, how does one go about constructing the expected value? Regardless, outside of the zip test above, I'd like to see more examples of each of these type of files and how they respond to the presence of such inputs in the gitignore. |
||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
|
||
@Test | ||
fun `test pattern with special characters`() { | ||
val input = "test[abc].txt" | ||
val expected = "(?:^|.*/?)test\\[abc\\]\\.txt(?:/.*)?$" | ||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
|
||
@Test | ||
fun `test pattern with double asterisk`() { | ||
val input = "**/build" | ||
val expected = "(?:^|.*/?).[^/]*[^/][^/]/build(?:/.*)?\$" | ||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
|
||
@Test | ||
fun `test pattern starting with slash`() { | ||
val input = "/root/file.txt" | ||
val expected = "^root/file\\.txt(?:/.*)?$" | ||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
|
||
@Test | ||
fun `test pattern ending with slash`() { | ||
val input = "build/" | ||
val expected = "(?:^|.*/?)build/.*" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these patterns have polynomial complexity. should there be a filter on input to prevent DoS of user's machine? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also can we investigate what it would look like to use the same glob logic that git uses to match files? seems like that might be simpler than trying to build these patterns There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added checks for polynomial complexity and makes sense for glob logic. will take a look into it. Wanted to drop regex matching entirely for /test. Only reason still being used is to restrict payload size which is leading to payload size errors. |
||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
|
||
@Test | ||
fun `test pattern with question mark`() { | ||
val input = "file?.txt" | ||
val expected = "(?:^|.*/?)file[^/]\\.txt(?:/.*)?$" | ||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
|
||
@Test | ||
fun `test complex pattern with multiple special characters`() { | ||
val input = "**/test-[0-9]*.{java,kt}" | ||
val expected = "(?:^|.*/?).[^/]*[^/][^/]/test-\\[0-9\\][^/]*[^/]\\.\\{java\\,kt\\}(?:/.*)?\$" | ||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
|
||
@Test | ||
fun `test empty pattern`() { | ||
val input = "" | ||
val expected = "(?:^|.*/?)(?:/.*)?$" | ||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
|
||
@Test | ||
fun `test pattern with all special regex characters`() { | ||
val input = ".$+()[]{}^|" | ||
val expected = "(?:^|.*/?)\\.\\\$\\+\\(\\)\\[\\]\\{\\}\\^\\|(?:/.*)?$" | ||
assertEquals(expected, featureDevSessionContext.convertGitIgnorePatternToRegex(input)) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -252,10 +252,40 @@ class FeatureDevSessionContext(val project: Project, val maxProjectSizeBytes: Lo | |
} | ||
|
||
// gitignore patterns are not regex, method update needed. | ||
private fun convertGitIgnorePatternToRegex(pattern: String): String = pattern | ||
fun convertGitIgnorePatternToRegex(pattern: String): String = pattern | ||
// Escape special regex characters except * and ? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added tests |
||
.replace(".", "\\.") | ||
.replace("*", ".*") | ||
.let { if (it.endsWith("/")) "$it.*" else "$it/.*" } // Add a trailing /* to all patterns. (we add a trailing / to all files when matching) | ||
.replace("+", "\\+") | ||
.replace("(", "\\(") | ||
.replace(")", "\\)") | ||
.replace("[", "\\[") | ||
.replace("]", "\\]") | ||
.replace("{", "\\{") | ||
.replace("}", "\\}") | ||
.replace(",", "\\,") | ||
.replace("^", "\\^") | ||
.replace("$", "\\$") | ||
.replace("|", "\\|") | ||
// Convert gitignore glob patterns to regex | ||
.replace("**", ".*?") // Match any directory depth | ||
.replace("*", "[^/]*?") // Match any character except path separator | ||
.replace("?", "[^/]") // Match single character except path separator | ||
.let { pattern -> | ||
when { | ||
// If pattern starts with '/', anchor it to the start of the path | ||
pattern.startsWith("/") -> "^${pattern.substring(1)}" | ||
// If pattern doesn't start with '/', it can match anywhere in the path | ||
else -> "(?:^|.*/?)$pattern" | ||
} | ||
} | ||
.let { pattern -> | ||
when { | ||
// If pattern ends with '/', it should match directories | ||
pattern.endsWith("/") -> "$pattern.*" | ||
// Otherwise match exactly or with a trailing slash for directories | ||
else -> "$pattern(?:/.*)?$" | ||
} | ||
} | ||
|
||
var selectedSourceFolder: VirtualFile | ||
set(newRoot) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that
settings.gradle
andbuild.gradle
are being dropped. These should be included as a part of the zip for /dev.Is this the result of changing the gitignore used for the test or a change in the actual behavior of the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can amend this for /dev.