Support flavors and test-suite options when no-build options is enabled #951
Conversation
WalkthroughThe changes in the pull request enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TestsCommand
participant RecordTests
participant PayloadHandler
User->>TestsCommand: Invoke tests command
TestsCommand->>RecordTests: Initialize session
RecordTests->>PayloadHandler: Prepare payload with testSuite and flavors
PayloadHandler-->>RecordTests: Return modified payload
RecordTests->>TestsCommand: Send payload for reporting
TestsCommand->>User: Provide feedback on test results
Possibly related PRs
Suggested reviewers
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}
| return { | ||
| "events": cs, | ||
| "testRunner": test_runner, | ||
| "group": group, | ||
| "metadata": get_env_values(client), | ||
| "noBuild": self.is_no_build, | ||
| # NOTE: | ||
| # testSuite and flavors are applied only when the no-build option is enabled | ||
| "testSuite": test_suite, | ||
| "flavors": flavor, | ||
| }, exs |
There was a problem hiding this comment.
Undefined variables test_suite and flavor in payload function
The variables test_suite and flavor used in the payload dictionary are not defined within the scope of the payload function. This will result in a NameError when the function is executed.
To fix this issue, you can either pass test_suite and flavor as parameters to the payload function or access them as instance variables of the RecordTests class.
Solution 1: Pass test_suite and flavor as parameters
Modify the payload function signature to include test_suite and flavor, and update the calls to this function.
def payload(cases: Generator[TestCase, None, None],
test_runner, group: str,
+ test_suite: Optional[str], flavor: Sequence[Tuple[str, str]]) -> Tuple[Dict[str, Union[str, List, dict, bool]], List[Exception]]:
# existing code
return {
"events": cs,
"testRunner": test_runner,
"group": group,
"metadata": get_env_values(client),
"noBuild": self.is_no_build,
# NOTE:
# testSuite and flavors are applied only when the no-build option is enabled
- "testSuite": test_suite,
- "flavors": flavor,
+ "testSuite": test_suite,
+ "flavors": flavor,
}, exsUpdate the calls to payload to include self.test_suite and self.flavor:
# Within the run method
- for chunk in ichunked(tc, post_chunk):
- p, es = payload(chunk, test_runner, group)
+ for chunk in ichunked(tc, post_chunk):
+ p, es = payload(chunk, test_runner, group, self.test_suite, self.flavor)Solution 2: Access test_suite and flavor as instance variables
Assign test_suite and flavor as instance variables in the RecordTests class and reference them using self in the payload function.
# In the __init__ method of RecordTests, add parameters and assign to instance variables
def __init__(self, dry_run=False,
+ test_suite: Optional[str] = None, flavor: Sequence[Tuple[str, str]] = ()):
# existing code
self.is_no_build = is_no_build
self.metadata_builder = CaseEvent.default_data_builder()
+ self.test_suite = test_suite
+ self.flavor = flavor
# In the payload function, reference instance variables
def payload(cases: Generator[TestCase, None, None],
test_runner, group: str) -> Tuple[Dict[str, Union[str, List, dict, bool]], List[Exception]]:
# existing code
return {
"events": cs,
"testRunner": test_runner,
"group": group,
"metadata": get_env_values(client),
"noBuild": self.is_no_build,
# NOTE:
# testSuite and flavors are applied only when the no-build option is enabled
- "testSuite": test_suite,
- "flavors": flavor,
+ "testSuite": self.test_suite,
+ "flavors": self.flavor,
}, exsEnsure that you pass test_suite and flavor when instantiating RecordTests:
# At the end of the tests function
-context.obj = RecordTests(dry_run=context.obj.dry_run)
+context.obj = RecordTests(dry_run=context.obj.dry_run, test_suite=test_suite, flavor=flavor)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
tests/data/behave/record_test_result.json (1)
28-30: LGTM! Consider adding documentation.The addition of
flavorsandtestSuitefields is consistent with the PR objectives. The initialization values are appropriate for backward compatibility.Consider documenting these new fields in the schema documentation or README to help users understand their purpose and usage.
tests/data/nunit/record_test_result-windows.json (1)
109-111: LGTM! Consider adding documentationThe implementation is consistent with other test runners. Since this is a new feature, consider documenting:
- The purpose and usage of
flavorsandtestSuitefields- How these fields interact with the
no-buildoptionWould you like me to help draft the documentation for these new fields?
tests/data/dotnet/record_test_result.json (1)
110-112: LGTM! Consider documenting the new fields.The addition of
flavorsandtestSuitefields is consistent with the PR objectives. The initialization values are appropriate for the test fixture.Consider adding a comment in the test file or documentation explaining the purpose and expected values of these new fields for future maintainers.
tests/data/cucumber/record_test_result.json (1)
Line range hint
62-64: Systematic enhancement of test result schemaThe consistent addition of
noBuild,flavors, andtestSuitefields across different test runner result files indicates a well-planned enhancement to support test suite configurations and flavor options. This systematic approach ensures uniform handling of these features across different test runners.Also applies to: 248-250, 292-294
tests/test_runners/test_raw.py (1)
288-289: LGTM! Consider adding test cases for non-empty values.The addition of
flavorsandtestSuitefields to the payload is correct. However, consider adding test cases that verify the behavior with non-empty flavors array and non-null testSuite value to ensure complete coverage.Example test case to add:
# Test with non-empty values result = self.cli('record', 'tests', '--flavor', 'unit', '--test-suite', 'regression', 'raw', test_path_file, mix_stderr=False) # Assert payload contains: # "flavors": ["unit"] # "testSuite": "regression"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (31)
tests/data/ant/record_test_result.json(1 hunks)tests/data/bazel/record_test_result.json(1 hunks)tests/data/bazel/record_test_with_build_event_json_result.json(1 hunks)tests/data/bazel/record_test_with_multiple_build_event_json_result.json(1 hunks)tests/data/behave/record_test_result.json(1 hunks)tests/data/ctest/record_test_result.json(1 hunks)tests/data/cts/record_test_result.json(1 hunks)tests/data/cucumber/record_test_json_result.json(1 hunks)tests/data/cucumber/record_test_result.json(1 hunks)tests/data/cypress/record_test_result.json(1 hunks)tests/data/dotnet/record_test_result.json(1 hunks)tests/data/flutter/record_test_result.json(1 hunks)tests/data/go_test/record_test_result.json(1 hunks)tests/data/googletest/fail/record_test_result.json(1 hunks)tests/data/googletest/record_test_result.json(1 hunks)tests/data/gradle/recursion/expected.json(1 hunks)tests/data/jest/record_test_result.json(1 hunks)tests/data/maven/record_test_result.json(1 hunks)tests/data/minitest/record_test_result.json(1 hunks)tests/data/nunit/nunit-reporter-bug-with-nested-type.json(1 hunks)tests/data/nunit/record_test_result-linux.json(1 hunks)tests/data/nunit/record_test_result-windows.json(1 hunks)tests/data/playwright/record_test_result.json(1 hunks)tests/data/playwright/record_test_result_with_json.json(1 hunks)tests/data/prove/record_test_result.json(1 hunks)tests/data/pytest/record_test_result.json(1 hunks)tests/data/pytest/record_test_result_json.json(1 hunks)tests/data/robot/record_test_executed_only_one_file_result.json(1 hunks)tests/data/robot/record_test_result.json(1 hunks)tests/data/rspec/record_test_result.json(1 hunks)tests/test_runners/test_raw.py(2 hunks)
✅ Files skipped from review due to trivial changes (18)
- tests/data/ant/record_test_result.json
- tests/data/robot/record_test_result.json
- tests/data/gradle/recursion/expected.json
- tests/data/go_test/record_test_result.json
- tests/data/cypress/record_test_result.json
- tests/data/jest/record_test_result.json
- tests/data/maven/record_test_result.json
- tests/data/googletest/fail/record_test_result.json
- tests/data/flutter/record_test_result.json
- tests/data/googletest/record_test_result.json
- tests/data/bazel/record_test_with_multiple_build_event_json_result.json
- tests/data/playwright/record_test_result.json
- tests/data/rspec/record_test_result.json
- tests/data/minitest/record_test_result.json
- tests/data/nunit/record_test_result-linux.json
- tests/data/pytest/record_test_result.json
- tests/data/playwright/record_test_result_with_json.json
- tests/data/cucumber/record_test_json_result.json
🔇 Additional comments (9)
tests/data/robot/record_test_executed_only_one_file_result.json (1)
60-62: LGTM! Consistent implementation.
The changes maintain consistency with other test result files, properly preserving the existing test event structure while adding the new fields.
tests/data/nunit/nunit-reporter-bug-with-nested-type.json (1)
39-41: LGTM! Verify nested type handling.
The changes are consistent with other test result files. Since this file specifically handles nested type cases, it would be good to verify that the new fields don't affect the nested type handling functionality.
Let's verify the nested type handling remains intact:
✅ Verification successful
Let me check the content of these files to verify the nested type handling consistency.
Nested type handling is properly maintained
After comparing the files, the nested type handling is consistent across all test files. The new fields (noBuild, flavors, testSuite) are uniformly applied and don't interfere with the nested type representation in the testPath arrays. Both Outer+Inner nested classes and parameterized tests are correctly structured in all files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files that handle nested types to ensure consistent structure
rg -l --type json "Outer\\+Inner|nested" tests/data/
Length of output: 207
Script:
#!/bin/bash
# Compare the structure and nested type handling across the identified files
echo "=== nunit-reporter-bug-with-nested-type.json ==="
cat tests/data/nunit/nunit-reporter-bug-with-nested-type.json
echo -e "\n=== record_test_result-windows.json ==="
cat tests/data/nunit/record_test_result-windows.json
echo -e "\n=== record_test_result-linux.json ==="
cat tests/data/nunit/record_test_result-linux.json
Length of output: 8062
tests/data/prove/record_test_result.json (1)
132-134: LGTM! Consistent with other test runners
The implementation maintains consistency with other test runners, using identical field structure and default values.
tests/data/pytest/record_test_result_json.json (1)
160-162: LGTM! Changes maintain consistency across test runners.
The addition of flavors and testSuite fields with matching default values ensures consistent schema across different test runners.
tests/data/bazel/record_test_result.json (1)
132-134: LGTM! Verify schema consistency across all test runners.
The changes maintain the consistent pattern of adding flavors and testSuite fields.
Let's verify that these schema changes are consistent across all test runner fixtures:
✅ Verification successful
Schema changes are consistent across all test runners ✅
The verification confirms that all test result fixtures have identical schema changes:
- All test runners have the
flavorsfield set to an empty array[] - All test runners have the
testSuitefield set tonull
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify schema consistency across all test result fixtures
# Expected: All test result files should have the new fields with matching default values
# Find all record_test_result*.json files
fd 'record_test_result.*\.json$' tests/data -x jq -r \
'select(has("flavors") and has("testSuite")) | "\(.testRunner): flavors=\(.flavors), testSuite=\(.testSuite)"'
Length of output: 1012
tests/data/ctest/record_test_result.json (1)
62-64: LGTM: New fields added consistently
The addition of noBuild, flavors, and testSuite fields maintains the JSON structure integrity and follows the expected schema changes.
tests/data/cts/record_test_result.json (1)
248-250: LGTM: Consistent field structure maintained
The new fields follow the same structure and default values as other test result files, maintaining consistency across different test runners.
tests/data/cucumber/record_test_result.json (1)
292-294: LGTM: Schema changes consistently applied
The new fields maintain consistency with other test result files. However, let's verify the schema validation in the main code.
✅ Verification successful
Based on the code analysis, I can now generate the final response since I have found clear evidence that the schema changes are properly handled in the codebase:
Schema changes are properly validated
The new fields (noBuild, flavors, testSuite) are properly handled in the test recording logic:
- The fields are part of the payload structure in
launchable/commands/record/tests.py - They are consistently used across all test runners through the
RecordTestsclass - The fields are properly sent to the API in the events payload
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify schema validation for new fields
# Search for schema validation or field processing code
rg -l "testSuite|flavors|noBuild" --type python
Length of output: 80
Script:
#!/bin/bash
# Let's first find all files that might contain schema validation
fd -e js -e ts -e json 'schema|test.*result' --type f
# Then search for the specific fields in those files
rg -l "testSuite|flavors|noBuild"
Length of output: 3103
Script:
#!/bin/bash
# Let's check the content of test_raw.py and record/tests.py for schema validation
cat tests/test_runners/test_raw.py
cat launchable/commands/record/tests.py
Length of output: 44890
tests/test_runners/test_raw.py (1)
367-369: LGTM! Verify behavior with no-build option.
The payload structure is correct. Since this PR is about supporting flavors and test-suite options when no-build is enabled, consider adding a test case that verifies the behavior when noBuild is True.
Let's verify if there are any existing tests for the no-build scenario:
Consider adding a test case:
# Test with no-build option
result = self.cli('record', 'tests', '--no-build', '--flavor', 'unit', '--test-suite', 'regression', 'raw', test_path_file, mix_stderr=False)
# Assert payload contains:
# "noBuild": True
# "flavors": ["unit"]
# "testSuite": "regression"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
launchable/commands/record/tests.py (1)
466-476: Add validation for test_suite parameterThe payload structure looks good, but consider adding validation for the
test_suiteparameter before using it in the payload. This will help prevent potential issues with invalid test suite names.Consider adding validation similar to the following:
return { "events": cs, "testRunner": test_runner, "group": group, "metadata": get_env_values(client), "noBuild": self.is_no_build, # NOTE: # testSuite and flavors are applied only when the no-build option is enabled - "testSuite": test_suite, + "testSuite": test_suite if test_suite and test_suite.strip() else None, "flavors": dict(flavor), }, exs
|
|
@ono-max you don't have to review carefully. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
launchable/commands/record/tests.py (1)
Line range hint
451-479: LGTM: Function signature and payload structure updated correctly.The payload function has been properly updated to handle test suite and flavors. Consider adding type hints for better code maintainability:
-def payload( - cases: Generator[TestCase, None, None], - test_runner, group: str, - test_suite_name: str, - flavors: Dict[str, str]) -> Tuple[Dict[str, Union[str, List, dict, bool]], List[Exception]]: +def payload( + cases: Generator[TestCase, None, None], + test_runner: str, + group: str, + test_suite_name: Optional[str], + flavors: Dict[str, str]) -> Tuple[Dict[str, Union[str, List, dict, bool]], List[Exception]]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (32)
launchable/commands/record/tests.py(3 hunks)tests/data/ant/record_test_result.json(1 hunks)tests/data/bazel/record_test_result.json(1 hunks)tests/data/bazel/record_test_with_build_event_json_result.json(1 hunks)tests/data/bazel/record_test_with_multiple_build_event_json_result.json(1 hunks)tests/data/behave/record_test_result.json(1 hunks)tests/data/ctest/record_test_result.json(1 hunks)tests/data/cts/record_test_result.json(1 hunks)tests/data/cucumber/record_test_json_result.json(1 hunks)tests/data/cucumber/record_test_result.json(1 hunks)tests/data/cypress/record_test_result.json(1 hunks)tests/data/dotnet/record_test_result.json(1 hunks)tests/data/flutter/record_test_result.json(1 hunks)tests/data/go_test/record_test_result.json(1 hunks)tests/data/googletest/fail/record_test_result.json(1 hunks)tests/data/googletest/record_test_result.json(1 hunks)tests/data/gradle/recursion/expected.json(1 hunks)tests/data/jest/record_test_result.json(1 hunks)tests/data/maven/record_test_result.json(1 hunks)tests/data/minitest/record_test_result.json(1 hunks)tests/data/nunit/nunit-reporter-bug-with-nested-type.json(1 hunks)tests/data/nunit/record_test_result-linux.json(1 hunks)tests/data/nunit/record_test_result-windows.json(1 hunks)tests/data/playwright/record_test_result.json(1 hunks)tests/data/playwright/record_test_result_with_json.json(1 hunks)tests/data/prove/record_test_result.json(1 hunks)tests/data/pytest/record_test_result.json(1 hunks)tests/data/pytest/record_test_result_json.json(1 hunks)tests/data/robot/record_test_executed_only_one_file_result.json(1 hunks)tests/data/robot/record_test_result.json(1 hunks)tests/data/rspec/record_test_result.json(1 hunks)tests/test_runners/test_raw.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (30)
- tests/data/robot/record_test_result.json
- tests/data/ant/record_test_result.json
- tests/data/maven/record_test_result.json
- tests/data/gradle/recursion/expected.json
- tests/data/nunit/record_test_result-linux.json
- tests/data/jest/record_test_result.json
- tests/data/bazel/record_test_with_build_event_json_result.json
- tests/data/cucumber/record_test_json_result.json
- tests/data/cucumber/record_test_result.json
- tests/data/behave/record_test_result.json
- tests/data/robot/record_test_executed_only_one_file_result.json
- tests/data/bazel/record_test_result.json
- tests/data/nunit/nunit-reporter-bug-with-nested-type.json
- tests/data/go_test/record_test_result.json
- tests/data/minitest/record_test_result.json
- tests/data/cypress/record_test_result.json
- tests/data/prove/record_test_result.json
- tests/data/nunit/record_test_result-windows.json
- tests/data/dotnet/record_test_result.json
- tests/data/bazel/record_test_with_multiple_build_event_json_result.json
- tests/data/googletest/fail/record_test_result.json
- tests/data/rspec/record_test_result.json
- tests/data/flutter/record_test_result.json
- tests/data/playwright/record_test_result.json
- tests/data/pytest/record_test_result.json
- tests/data/googletest/record_test_result.json
- tests/data/pytest/record_test_result_json.json
- tests/data/playwright/record_test_result_with_json.json
- tests/data/cts/record_test_result.json
- tests/data/ctest/record_test_result.json
🔇 Additional comments (3)
tests/test_runners/test_raw.py (2)
288-289: LGTM: Test payload updated correctly.
The test payload has been properly updated to include the new fields with appropriate default values.
367-369: LGTM: Consistent test payload update.
The test payload in the junit XML test case has been updated consistently with the same default values.
launchable/commands/record/tests.py (1)
548-554: LGTM: Payload function call updated correctly.
The payload function is called with appropriate parameters:
- Empty string as default for test_suite is handled properly
- Flavor tuple is correctly converted to dict
SSIA
Summary by CodeRabbit
New Features
testSuiteandflavors.Bug Fixes
Improvements