Conversation
chreekat
left a comment
There was a problem hiding this comment.
I have pointed out a few typical mistakes made by agents of the current era. There are more to be found. I suggest taking a look and trying to improve its output.
| [ (CmdTest.noTestsProblem, mkTargetPackage "p-0.1") | ||
| , (CmdTest.noTestsProblem, mkTargetPackage "q-0.1") | ||
| ] | ||
| do |
There was a problem hiding this comment.
I don't understand why this test changed. The inline comment on new line 1422 doesn't explain it. At most, it makes it sound like this test has gotten worse by testing the same thing as the new test.
| issues: 11858 | ||
| --- | ||
|
|
||
| `cabal test` no longer skips all tests when some of the requested targets refer |
There was a problem hiding this comment.
This whole changelog entry is too long. Probably 10x too long in terms of lines.
| Nothing | ||
| targetSelectors | ||
|
|
||
| let noTestsSelectors = |
There was a problem hiding this comment.
Previously the noticing or dying happened inside reportTargetProblems. I think that should continue to be the case, rather than splatting it into a higher level.
| -- For the @test@ command we select all buildable test-suites, | ||
| -- or fail if there are no test-suites or no buildable test-suites. | ||
| -- For the @test@ command we select all buildable test-suites. | ||
| -- A target that contains no test-suites does not select anything: it is |
There was a problem hiding this comment.
This comment edit is sloppy. It doesn't seem to be telling me anything important about this function in particular. It says a target is skipped with a notice, but there is no notice happening here? And there's no commit message explaining why this function changed. It's hard to understand the intent.
|
Sorry to rain on your parade with this review. I do appreciate that you tackled the issue I raised! The new test looks fine; if the impl was cleaned up I'm sure this would actually fix my problem. |
fix: #11858
Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significantin the changelog file.