Currently, if a probe fails, it will print the line number of the [test] block the probe is in, not the line number of the probe itself. This makes it somewhat difficult to debug.
This commit makes it print the line number of the probe as well.
CC @zfigura
-- v4: tests: Print the line number of the probe when a test fails.
From: Petrichor Park ppark@codeweavers.com
Currently, if a probe fails, it will print the line number of the [test] block the probe is in, not the line number of the probe itself. This makes it somewhat difficult to debug.
This commit makes it print the line number of the probe as well. --- tests/shader_runner.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 0e6104ce3..5ad84cf3a 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -533,7 +533,7 @@ static void read_uint64_t2(const char **line, struct u64vec2 *v) read_uint64(line, &v->y); }
-static void parse_test_directive(struct shader_runner *runner, const char *line) +static void parse_test_directive(struct shader_runner *runner, const char *line, unsigned int line_number) { char *rest; int ret; @@ -658,6 +658,8 @@ static void parse_test_directive(struct shader_runner *runner, const char *line) if (runner->last_render_failed) return;
+ vkd3d_test_push_context("probe line %u", line_number); + if (match_string(line, "uav", &line)) { slot = strtoul(line, &rest, 10); @@ -742,6 +744,7 @@ static void parse_test_directive(struct shader_runner *runner, const char *line) }
runner->ops->release_readback(runner, rb); + vkd3d_test_pop_context(); } else if (match_string(line, "uniform", &line)) { @@ -1433,7 +1436,8 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o runner->input_element_count = 0; }
- vkd3d_test_push_context("Section %.*s, line %u", strlen(line_buffer) - 1, line_buffer, line_number); + if (state != STATE_TEST) + vkd3d_test_push_context("Section %.*s, line %u", strlen(line_buffer) - 1, line_buffer, line_number); } else if (line[0] != '%' && line[0] != '\n') { @@ -1482,9 +1486,12 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o /* Compilation which fails with dxcompiler is not 'todo', therefore the tests are * not 'todo' either. They cannot run, so skip them entirely. */ if (!skip_tests && SUCCEEDED(expect_hr)) - parse_test_directive(runner, line); + parse_test_directive(runner, line, line_number); break; } + + if (state != STATE_TEST) + vkd3d_test_pop_context(); } }
On Mon Dec 4 17:00:38 2023 +0000, Henri Verbeet wrote:
Shouldn't this simply be a matter of adjusting the existing vkd3d_test_push_context() and vkd3d_test_pop_context() calls in run_shader_tests() to update the context for each line?
Maybe? It would require a whole lot of rewriting of that function, and it's so messy I think it's probably easier to just monkey-patch it like this.
Maybe? It would require a whole lot of rewriting of that function, and it's so messy I think it's probably easier to just monkey-patch it like this.
I may be overlooking the obvious, but it doesn't seem that complicated. E.g., something like the following seems to work for me at first sight:
```diff diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 0e6104ce..f94e3309 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -1121,6 +1121,7 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o HRESULT expect_hr = S_OK; bool skip_tests = false; char line_buffer[256]; + const char *testname; FILE *f;
if (!test_options.filename) @@ -1134,12 +1135,19 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o runner->minimum_shader_model = minimum_shader_model; runner->maximum_shader_model = maximum_shader_model;
+ if ((testname = strrchr(test_options.filename, '/'))) + ++testname; + else + testname = test_options.filename; + for (;;) { char *ret = fgets(line_buffer, sizeof(line_buffer), f); const char *line = line_buffer;
- ++line_number; + if (line_number++) + vkd3d_test_pop_context(); + vkd3d_test_push_context("%s:%u", testname, line_number);
if (!ret || line[0] == '[') { @@ -1289,9 +1297,6 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o } }
- if (state != STATE_NONE) - vkd3d_test_pop_context(); - if (!ret) break; } @@ -1432,8 +1437,6 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o free(runner->input_elements[i].name); runner->input_element_count = 0; } - - vkd3d_test_push_context("Section %.*s, line %u", strlen(line_buffer) - 1, line_buffer, line_number); } else if (line[0] != '%' && line[0] != '\n') { @@ -1488,6 +1491,9 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o } }
+ if (line_number) + vkd3d_test_pop_context(); + for (i = 0; i < runner->input_element_count; ++i) free(runner->input_elements[i].name); free(runner->input_elements); ```
On Tue Dec 5 12:27:26 2023 +0000, Henri Verbeet wrote:
Maybe? It would require a whole lot of rewriting of that function, and
it's so messy I think it's probably easier to just monkey-patch it like this. I may be overlooking the obvious, but it doesn't seem that complicated. E.g., something like the following seems to work for me at first sight:
diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 0e6104ce..f94e3309 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -1121,6 +1121,7 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o HRESULT expect_hr = S_OK; bool skip_tests = false; char line_buffer[256]; + const char *testname; FILE *f; if (!test_options.filename) @@ -1134,12 +1135,19 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o runner->minimum_shader_model = minimum_shader_model; runner->maximum_shader_model = maximum_shader_model; + if ((testname = strrchr(test_options.filename, '/'))) + ++testname; + else + testname = test_options.filename; + for (;;) { char *ret = fgets(line_buffer, sizeof(line_buffer), f); const char *line = line_buffer; - ++line_number; + if (line_number++) + vkd3d_test_pop_context(); + vkd3d_test_push_context("%s:%u", testname, line_number); if (!ret || line[0] == '[') { @@ -1289,9 +1297,6 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o } } - if (state != STATE_NONE) - vkd3d_test_pop_context(); - if (!ret) break; } @@ -1432,8 +1437,6 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o free(runner->input_elements[i].name); runner->input_element_count = 0; } - - vkd3d_test_push_context("Section %.*s, line %u", strlen(line_buffer) - 1, line_buffer, line_number); } else if (line[0] != '%' && line[0] != '\n') { @@ -1488,6 +1491,9 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o } } + if (line_number) + vkd3d_test_pop_context(); + for (i = 0; i < runner->input_element_count; ++i) free(runner->input_elements[i].name); free(runner->input_elements);
Thanks! (This is my first time looking at the test harness code so I'm not super sure how it works.)
On Thu Dec 7 16:17:35 2023 +0000, Petrichor Park wrote:
Thanks! (This is my first time looking at the test harness code so I'm not super sure how it works.)
That works great. I'm going to open that as another MR...
This merge request was closed by Petrichor Park.
Gonna close this in favor of !516
-
Henri Verbeet (＠hverbeet) -
Petrichor Park
-
Petrichor Park (＠petrathekat)