Handle skipped tests in "Not run" column - #3743
Conversation
williamjallen
left a comment
williamjallen
left a comment
There was a problem hiding this comment.
I'm conflicted on whether this belongs in CDash, or whether this should be added as a CTest property. I'm leaning towards the latter because it's possible that some projects will want to dynamically set the list of not run test warnings. The build error filter feature removed in #3650 had roughly the same considerations. Any thoughts on this?
|
It could be in addition to having a "Skipped" test property in CTest. However in CDash we would need a way to handle the display of skipped tests. Where you thinking something else? |
|
@jjomier Is your goal just to differentiate between disabled tests and tests which were "not run" for a different reason? We want to minimize the number of configuration options if possible, so I'm hesitant to approve this as-is. After discussing this internally, we think the best approach is to just match the text "Disabled" in the details field without it being user-configurable. Thoughts? |
|
@williamjallen yes the idea is to not pollute the tests that are marked as not run when we look at the dashboard. I like the idea of matching "Disabled". |
ef9cd03 to
fc65763
Compare
This commit introduces a new `notRunSkippedDetailsRegex` property to the Project model, allowing users to define regex patterns for not-run test details. A corresponding validation rule is added to ensure the regex is valid. The Build model now includes a method to count not-run tests that do not match the specified patterns, and the GraphQL schema is updated to expose this functionality. Additionally, UI components are modified to display the warning count and adjust test status colors based on the new regex patterns. Tests are also added to verify the new functionality.
This commit removes the `notRunSkippedDetailsRegex` property from the Project model and associated validation rule, simplifying the logic for handling not-run tests. The `notRunTestsWarningCount` method in the Build model is updated to exclude tests with details marked as "Disabled". The GraphQL schema and UI components are adjusted accordingly to reflect these changes. Additionally, tests are updated to ensure correct behavior without the regex pattern handling.
This commit improves the formatting of the `testStatusToColorClass` and `testStatusToTextColorClass` functions in the TestDisplay component. The switch cases have been reformatted for better readability, maintaining the same functionality. No changes to logic or behavior were made.
This commit simplifies the `notRunTestsWarningCount` method in the Build model by using a more efficient query to count not-run tests that do not have details marked as "Disabled". Additionally, the API response in the Index controller is updated to reflect the new method signature. Unused test methods in the TestDisplayTest class have been removed to clean up the codebase.
Replace the App\Utils\TestDisplay helper with a notRunWarning() query scope on the Test model, so the "Disabled" completion status is interpreted in one place next to the data it describes. The status color class is derived directly from the existing Test::DISABLED constant where it is needed, and the unused text/GraphQL color class helpers are dropped.
Counting every test of every build was only viable for small instances, since the index page renders many builds at once. Record the number of not-run tests worth warning about in a new build.testnotrunwarning column as test results are submitted, and read that column instead, so the index page cost no longer scales with the number of tests.
Nothing links to a specific settings section now that the not-run details are matched without a per-project option, so the anchor prop on FormSection and the hash navigation it existed for are unused.
The text-color mapping was leftover from the old PHP TestDisplay utility and is not imported anywhere in the Vue UI.
Master moved the configure/build/test status colors onto BuildTimelineCard. Use notRunTestsWarningCount there so tests whose details are "Disabled" do not turn the Test step orange.
3fda663 to
a887248
Compare
|
@williamjallen I just fixed phpstan issue, we should be good. |
|
@williamjallen unsure if the current tests are due to this PR changes - seems unrelated. Do you have an idea? |
|
@jjomier Both of the failing tests are known to be flaky. I just re-ran them for you. I'm planning to review this PR later this afternoon. |
|
Thank you @williamjallen! Feel free to edit this PR if there are minor changes. |
| self::assertTrue($browser->script( | ||
| 'return document.querySelector(\'a[href*="/tests/' . $disabled_not_run_test->id . '"]\')' | ||
| . '?.closest("tr")?.querySelector("td.normal") !== null', | ||
| )[0]); | ||
|
|
||
| self::assertTrue($browser->script( | ||
| 'return document.querySelector(\'a[href*="/tests/' . $warning_not_run_test->id . '"]\')' | ||
| . '?.closest("tr")?.querySelector("td.warning") !== null', | ||
| )[0]); |
There was a problem hiding this comment.
This needs to be rewritten to use data-test selectors. We try to avoid using ->script() as much as possible.
| ]); | ||
| } | ||
|
|
||
| public function testNotRunTestsWarningCount(): void |
There was a problem hiding this comment.
This test case should be removed, and the functionality should be replaced by specifying the field in testBasicFieldAccess above.
| $numberTestsNotRunWarning = Test::where('buildid', (int) $this->Id) | ||
| ->notRunWarning() | ||
| ->count(); |
There was a problem hiding this comment.
You should use whereHas() or similar to avoid needing to specify the buildid column name. Ideally, there should only ever be one source of truth about column names: the names specified in the model.
This commit updates the Build model to ensure the count of not-run tests with warnings is correctly handled, defaulting to zero when no tests are found. In the Vue components, a new `testId` attribute is added for better identification of test statuses, and the DataTable component is modified to conditionally set the `data-test` attribute based on the presence of this new attribute. Additionally, the BuildTypeTest is updated to include the `notRunTestsWarningCount` in the GraphQL response, ensuring accurate data representation in tests.
|
@williamjallen thinking a little bit about this PR, we should probably add also the "SKIP_*" in the list of tests to display green. The main reason is if we explicitly know we want to skip the test it should not be shown as a warning. I'm open to other suggestions. |
2 participants
This commit introduces a new
notRunSkippedDetailsRegexproperty to the Project model, allowing users to define regex patterns for not-run test details. A corresponding validation rule is added to ensure the regex is valid. The Build model now includes a method to count not-run tests that do not match the specified patterns, and the GraphQL schema is updated to expose this functionality. Additionally, UI components are modified to display the warning count and adjust test status colors based on the new regex patterns. Tests are also added to verify the new functionality.