Skip to content
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

test group filter, run retries #638

Merged
merged 5 commits into from
Aug 21, 2024
Merged

test group filter, run retries #638

merged 5 commits into from
Aug 21, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 21, 2024

Making the test suite more robust

  • 🔄 The signature of the command-line options have been updated slightly:
    • csv-separator now supports a shorthand flag -cs
    • A new run-retry flag has been added with shorthand -rr.
  • 🎲 Retry logic has been introduced for script execution. In case a script fails, it is automatically retried based on the number of retries specified in run-retry.
  • 🏷️ Group tagging has been added—and is supported via the groups flag—to perform categorical filter operations. Filtering can be reversed by using the :! prefix.
  • 👁️ Visual group scripts have been introduced and are distinguished with a vision tag.
  • ➕ The command-line scripts now include an additional external library in the form "packages/sample/genaisrc/describe-image-run-prompt.genai.js".
  • 🚀 Improvement in error logs to distinguish the message from serialized error.
  • 🔄 Small updates in test scripts and command-line tool for applying rudimentary tag filtering and retry logic.
  • 📊 Changes to the packages/sample's package.json to exclude vision tests during script testing.
  • 💽 Minor optimizations such as normalization of variables, and improvements in the readability of the code.

generated by pr-describe

packages/core/src/util.ts Outdated Show resolved Hide resolved
Copy link

The pull request introduces several changes:

  1. In cli.ts, two new options are added: --run-retry for specifying the number of retries for the entire run, and --groups which allows to include or exclude groups.
  2. The retry logic is implemented in run.ts. If the exitCode is 0, it breaks the retry loop.
  3. In test.ts, a filter is added to apply the group option during test execution.
  4. The PromptScriptTestRunOptions and PromptScriptRunOptions interfaces in messages.ts have been extended with groups and runRetry respectively.
  5. A new function tagFilter is added in util.ts which checks if a tag exists and can be filtered.

Concerns:

  • The tagFilter function may not work correctly if tags are not in lower case as it uses toLocaleLowerCase for comparison. If the tags' case doesn't matter, this could potentially be an issue.
  • The retry logic in runScriptWithExitCode could potentially result in an infinite loop if runRetry is not properly validated. The current implementation only ensures that runRetry is at least 1.

Additional suggestions:

@@ -79,7 +79,14 @@ export async function runScriptWithExitCode(
 ) {
-    const runRetry = Math.max(1, normalizeInt(options.runRetry) || 1)
+    const runRetry = Math.max(1, normalizeInt(options.runRetry || 1))
     let exitCode = -1
     for (let r = 0; r < runRetry; ++r) {
         const res = await runScript(scriptId, files, options)
         exitCode = res.exitCode
         if (exitCode === 0) break
         console.error(`run failed, retrying ${r + 1}/${runRetry}`)
     }
     process.exit(exitCode)
 }

This is to ensure that options.runRetry is not undefined when it is passed to normalizeInt.

@@ -285,3 +285,14 @@ export function renderWithPrecision(
+
+export function tagFilter(tags: string[], tag: string) {
+    if (!tags?.length || !tag) return true
+    const ltag = tag.toLocaleLowerCase()
+    for (const t of tags) {
+        const lt = t.toLocaleLowerCase()
+        if (lt.startsWith(":!") && ltag.startsWith(lt.slice(2))) return false
+        else if (ltag.startsWith(t)) return true
+    }
+    return false
+}

I suggest validating the case of the tags before comparison, or use a case-insensitive comparison method if available.

Other than these minor issues, the changes look good to me. LGTM 🚀

generated by pr-review

USER_CANCELLED_ERROR_CODE,
FILES_NOT_FOUND_ERROR_CODE,
ANNOTATION_ERROR_CODE,
])

Choose a reason for hiding this comment

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

The UNRECOVERABLE_ERROR_CODES constant includes 0 which is typically a success error code. This could lead to incorrect behavior if a successful operation is treated as an unrecoverable error.

generated by pr-review-commit unrecoverable_error_codes

.option("-ae, --apply-edits", "apply file edits")
.option(
"--vars <namevalue...>",
"variables, as name=value, stored in env.vars"
)
.option("-rr, --run-retry <number>", "number of retries for the entire run")

Choose a reason for hiding this comment

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

The option "-rr, --run-retry " is missing a description.

generated by pr-review-commit missing_argument

.option(
"--groups <groups...>",
"groups to include or exclude. Use :! prefix to exclude"
)
.action(scriptsTest)

Choose a reason for hiding this comment

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

The option "--groups <groups...>" is missing a description.

generated by pr-review-commit missing_argument

@pelikhan pelikhan merged commit 26c511d into main Aug 21, 2024
6 of 8 checks passed
@pelikhan pelikhan deleted the test-group-filter branch August 21, 2024 16:16
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.

1 participant