Update default-specs.js validation params
#8924
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes focus on the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/cypress/support/default-specs.js (1)
Add test coverage for new validation parameters
The following validation parameters lack test coverage:
frameShareframesPerJobCountframesPerJobSharePlease add test cases in
tests/cypress/e2e/features/ground_truth_jobs.jsto verify these parameters, following the existing test structure in the "Testing creating task with quality params" describe block.🔗 Analysis chain
Line range hint
1-85: Verify test coverage for new validation parameters.The PR objectives indicate that tests haven't been addressed. Let's verify the test coverage for the new validation parameters:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests using the new validation parameters rg -l "frameShare|framesPerJobCount|framesPerJobShare" --type js tests/cypress/e2e/Length of output: 85
Script:
#!/bin/bash # Search for validation-related tests more broadly rg -l "validation.*params|validationParams" --type js tests/ # Search for any test files containing "validation" to ensure we don't miss anything rg -l "validation" --type js tests/Length of output: 481
Script:
#!/bin/bash # Check content of ground truth jobs test file cat tests/cypress/e2e/features/ground_truth_jobs.js # Also search for specific usage of validation parameters in this file rg "validationParams" -A 5 -B 5 tests/cypress/e2e/features/ground_truth_jobs.jsLength of output: 17658
🧹 Nitpick comments (3)
tests/cypress/support/default-specs.js (3)
46-57: Add parameter validation and documentation.While the parameter processing is consistent, there's no validation of the parameter values or documentation about their constraints. Consider:
- Adding JSDoc comments describing each parameter's purpose, constraints, and valid values
- Adding runtime validation for critical parameters
Example improvement:
/** * @typedef {Object} ValidationParams * @property {'gt_pool'|'expert_pool'} mode - The validation mode * @property {number} [randomSeed] - Seed for random frame selection * @property {number[]} [frames] - Specific frames to validate * ... */ // Add parameter validation if (validationParams.mode && !['gt_pool', 'expert_pool'].includes(validationParams.mode)) { throw new Error('Invalid validation mode'); }
Line range hint
5-13: Add TypeScript or JSDoc type definitions.The function lacks type definitions which could help prevent runtime errors and improve maintainability. Consider:
- Adding TypeScript types or JSDoc type definitions
- Providing default values for optional parameters
Example improvement:
/** * Creates a default task specification for testing. * @param {Object} params - The task parameters * @param {string} params.labelName - The name of the label * @param {string} [params.labelType='any'] - The type of the label * @param {string} params.taskName - The name of the task * @param {string[]} params.serverFiles - Array of server file paths * @param {number} [params.startFrame] - Starting frame number * @param {string} [params.frameFilter] - Frame filter expression * @param {number} [params.segmentSize] - Size of each segment * @param {ValidationParams} [params.validationParams] - Validation parameters * @returns {Object} The task specification object */
Line range hint
23-31: Extract sorting method logic to a separate function.The sorting method determination is embedded in the dataSpec object. Consider extracting this logic to improve readability and testability:
function determineSortingMethod(validationParams) { return (validationParams?.mode === 'gt_pool') ? 'random' : 'lexicographical'; } const dataSpec = { ... sorting_method: determineSortingMethod(validationParams), ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cypress/support/default-specs.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build
- GitHub Check: generate_github_pages
- GitHub Check: testing
- GitHub Check: Analyze (python)
- GitHub Check: Linter
- GitHub Check: Analyze (javascript)
| if (validationParams.frameShare) { | ||
| convertedParams.frame_share = validationParams.frameShare; | ||
| } | ||
| if (validationParams.framesPerJobCount) { | ||
| convertedParams.frames_per_job_count = validationParams.framesPerJobCount; | ||
| } | ||
| if (validationParams.mode) { | ||
| convertedParams.mode = validationParams.mode; | ||
| } | ||
| if (validationParams.randomSeed) { | ||
| convertedParams.random_seed = validationParams.randomSeed; | ||
| if (validationParams.framesPerJobShare) { | ||
| convertedParams.frames_per_job_share = validationParams.framesPerJobShare; |
There was a problem hiding this comment.
💡 Codebase verification
Parameters are mutually exclusive based on frame selection method
Based on the API schema, these parameters should be used as follows:
- For random uniform selection: use either
frameShare(percentage) orframeCount(absolute) - For random per-job selection: use either
framesPerJobShare(percentage) orframesPerJobCount(absolute) - Never combine percentage-based parameters with count-based parameters
🔗 Analysis chain
Consider mutual exclusivity of parameters.
The new parameters frameShare, framesPerJobCount, and framesPerJobShare might be mutually exclusive. Consider:
- Adding validation to prevent conflicting parameter combinations
- Documenting which parameters can be used together
Let's verify the parameter usage in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for examples of validation parameter usage to understand valid combinations
rg -A 5 "validation_params.*frame" --type js
Length of output: 46
Script:
#!/bin/bash
# Search for any usage of these parameters in both camelCase and snake_case forms
rg -i "frameShare|frame_share" --type js
rg -i "framesPerJobCount|frames_per_job_count" --type js
rg -i "framesPerJobShare|frames_per_job_share" --type js
# Search for test cases or specs that might demonstrate parameter combinations
fd -e spec.js -e test.js -e cy.js -X rg -l "frame"
# Look for validation logic
ast-grep --pattern 'if ($PARAM) { $$$_BODY }' tests/cypress/support/default-specs.js
Length of output: 3895
Script:
#!/bin/bash
# Search for documentation about these parameters
fd -e md -X rg -l "frame.*share|frames.*job"
# Look for any configuration or schema files that might define parameter relationships
fd -e json -e yaml -e yml -X rg -l "frame.*share|frames.*job"
# Search for test cases that might show parameter combinations
rg -B 5 -A 5 "validation_params.*=.*{" --type js
Length of output: 1515
Script:
#!/bin/bash
# Check schema file for parameter definitions
cat cvat/schema.yml
# Look for test examples with correct syntax
rg -B 5 -A 5 "validation_params.*=" --type js
# Check specific documentation files that might be relevant
cat site/content/en/docs/api_sdk/cli/_index.md
cat site/content/en/docs/manual/advanced/specification.md
Length of output: 65804
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8924 +/- ##
===========================================
- Coverage 73.84% 73.83% -0.02%
===========================================
Files 412 412
Lines 44261 44261
Branches 3993 3993
===========================================
- Hits 32686 32680 -6
- Misses 11575 11581 +6
|
|
Motivation and context
Updated default specs file with params as some of them are not present
How has this been tested?
Checklist
developbranch[] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Tests