-
Notifications
You must be signed in to change notification settings - Fork 246
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
feat(cli): execute one test from a test file #338
feat(cli): execute one test from a test file #338
Conversation
@khalatevarun is attempting to deploy a commit to the Antiwork Team on Vercel. A member of the Team first needs to authorize it. |
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.
I'll be able to take a closer look on this during the next milestone (next week or so). Generally, I think it should use a parser like @babel/parser
as it is more reliable than regex.
Here is a sample AI implementation, for reference/inspiration (haven't looked into details nor tested the code)
…varun/shortest into feat/execute-test-lineno
@rmarescu I request you to please take a look at this PR as its ready and synced with latest development |
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.
It appears that Babel would be a better option for us.
- TypeScript/JSX Support: Since Shortest is written in TypeScript, Babel's native TypeScript parsing will more accurately handle test files.
- Parent-Child Relationship Tracking: The parentPath feature in Babel's traversal is ideal for following method chains - essential for our use case where
shortest()
calls have chainedexpect()
methods. - Scope Awareness: Babel can identify related test assertions even when they span multiple lines or appear in different expressions but the same scope.
- Visitor Pattern: Babel's visitor pattern is more efficient than current nested AST walks.
- Type Safety: Better TypeScript types for AST nodes will make your code more maintainable.
Although Acorn generally has better performance metrics:
- The performance difference is minimal for our specific use case
- We're parsing individual test files, not entire codebases
- The improved accuracy in test detection far outweighs any slight performance difference
For example, the current logic doesn't parse the updated api-assert-bearer.test.ts
correctly:
Test Discovery: No tests found at line 7 in examples/api-assert-bearer.test.ts
Will work on replacing with Babel in this PR, and will add TestCase
in a separate one.
|
||
if (lineNumber) { | ||
testsToRun = this.filterTestsByLineNumber( | ||
registry.currentFileTests, |
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.
I think each TestFunction
should have the info about startLineNumber
and endLineNumber
so that, together with the filePath
, should generate a more unique hash for the test.
Right now, filePath
on TestFunction
is not set, which means the hash is generated mostly from the name
shortest/packages/shortest/src/index.ts
Lines 131 to 135 in 1dba67e
const test: TestFunction = { | |
name, | |
filePath: "", | |
expectations: [], | |
}; |
That can cause conflicts when 2 tests have the same name. The combo of name + filePath + startLineNumber + endLineNumber should create a unique hash.
With that in place, the logic here would only need to filter tests that fit the criteria.
I'll implement in a follow-up PR.
@@ -281,16 +285,78 @@ export class TestRunner { | |||
}; | |||
} | |||
|
|||
private async executeTestFile(file: string) { | |||
private async filterTestsByLineNumber( |
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.
Not that straight-forward to add test coverate for this at this moment.
if (!testLocation) { | ||
testLocation = testLocations.find((location) => { | ||
const TEMP_TOKEN = "##PLACEHOLDER##"; | ||
let pattern = location.testName.replace( | ||
new RegExp(escapeRegex(EXPRESSION_PLACEHOLDER), "g"), | ||
TEMP_TOKEN, | ||
); | ||
|
||
pattern = escapeRegex(pattern); | ||
pattern = pattern.replace(new RegExp(TEMP_TOKEN, "g"), ".*"); | ||
const regex = new RegExp(`^${pattern}$`); | ||
|
||
return regex.test(testNameNormalized); | ||
}); | ||
} |
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.
parseShortestTestFile
is used on TS test files, while the name stored on TestFunction
object is generated from the compiled file. Because of that, the names may not match.
E.g. in TS file:
Test the API POST endpoint ${TESTING_API_BASE_URI}/assert-bearer with body { "flagged": "true" } and the bearer token ${ALLOWED_TEST_BEARER}
Compiled file (expressions evaluated):
Test the API POST endpoint api/assert-bearer with body { "flagged": "true" } and the bearer token Bearer 123
fix: #281
Affected regions:
runTests()
executeTestFile()
main()
acorn
andacorn-walk