-
Notifications
You must be signed in to change notification settings - Fork 6k
Started executing vulkan unit tests with validation on macos #42337
Changes from all commits
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 |
|---|---|---|
|
|
@@ -75,19 +75,23 @@ def run_cmd( | |
| print(f'Running command "{command_string}"') | ||
|
|
||
| start_time = time.time() | ||
| collect_output = forbidden_output or allowed_failure_output | ||
| stdout_pipe = sys.stdout if not collect_output else subprocess.PIPE | ||
| stderr_pipe = sys.stderr if not collect_output else subprocess.PIPE | ||
|
|
||
| process = subprocess.Popen( | ||
| cmd, | ||
| stdout=stdout_pipe, | ||
| stderr=stderr_pipe, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.STDOUT, | ||
| env=env, | ||
| universal_newlines=True, | ||
| **kwargs | ||
| ) | ||
| stdout, stderr = process.communicate() | ||
| output = '' | ||
|
|
||
| for line in iter(process.stdout.readline, ''): | ||
| output += line | ||
| sys.stdout.write(line) | ||
|
Member
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. Previously the test script would only generate output when a test fails. This is nice when diagnosing failures on CI since the succeeding tests add only minimal output to the logs. Am I misreading what this does? Is there some way to make this configurable?
Member
Author
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. I believe you are misremembering how it works. If |
||
|
|
||
| sys.stdout.flush() | ||
| process.wait() | ||
|
Member
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. Does
Member
Author
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. That's my understanding. It's reading the pipe until EOF so there should be no more waiting on the pipe by the time we call |
||
| end_time = time.time() | ||
|
|
||
| if process.returncode != 0 and not expect_failure: | ||
|
|
@@ -97,32 +101,20 @@ def run_cmd( | |
| f'Failed Command:\n\n{command_string}\n\nExit Code: {process.returncode}\n' | ||
| ) | ||
|
|
||
| if stdout: | ||
| print(f'STDOUT: \n{stdout}') | ||
|
|
||
| if stderr: | ||
| print(f'STDERR: \n{stderr}') | ||
|
|
||
| print_divider('!') | ||
|
|
||
| allowed_failure = False | ||
| for allowed_string in allowed_failure_output: | ||
| if (stdout and allowed_string in stdout) or (stderr and | ||
| allowed_string in stderr): | ||
| if allowed_string in output: | ||
| allowed_failure = True | ||
|
|
||
| if not allowed_failure: | ||
| raise RuntimeError( | ||
| f'Command "{command_string}" exited with code {process.returncode}.' | ||
| ) | ||
|
|
||
| if stdout or stderr: | ||
| print(stdout) | ||
| print(stderr) | ||
|
|
||
| for forbidden_string in forbidden_output: | ||
| if (stdout and forbidden_string in stdout) or (stderr and | ||
| forbidden_string in stderr): | ||
| if forbidden_string in output: | ||
| raise RuntimeError( | ||
| f'command "{command_string}" contained forbidden string {forbidden_string}' | ||
| ) | ||
|
|
@@ -489,7 +481,12 @@ def make_test(name, flags=None, extra_env=None): | |
| '1', # Validates accesses to threadgroup memory. | ||
| 'MTL_SHADER_VALIDATION_TEXTURE_USAGE': | ||
| '1', # Validates that texture references are not nil. | ||
| # Note: built from //third_party/swiftshader | ||
| 'VK_ICD_FILENAMES': os.path.join(build_dir, 'vk_swiftshader_icd.json'), | ||
| # Note: built from //third_party/vulkan_validation_layers:vulkan_gen_json_files | ||
| # and //third_party/vulkan_validation_layers. | ||
| 'VK_LAYER_PATH': os.path.join(build_dir, 'vulkan-data'), | ||
|
Member
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. A comment pointing to the GN file with the string that has to be the same as this one would be good?
Member
Author
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. done |
||
| 'VK_INSTANCE_LAYERS': 'VK_LAYER_KHRONOS_validation', | ||
| } | ||
| if is_aarm64(): | ||
| extra_env.update({ | ||
|
|
@@ -501,7 +498,7 @@ def make_test(name, flags=None, extra_env=None): | |
| build_dir, | ||
| 'impeller_unittests', | ||
| executable_filter, | ||
| shuffle_flags, | ||
| shuffle_flags + ['--enable_vulkan_validation'], | ||
| coverage=coverage, | ||
| extra_env=extra_env, | ||
| # TODO(117122): Remove this allowlist. | ||
|
|
||
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.
This change allows us to stream the output to stdout but also capture it for inspection. The tests take a long enough time, it's a pain not seeing the streaming results.