array-api-tests icon indicating copy to clipboard operation
array-api-tests copied to clipboard

TST: Only skip/xfail tests whose ids exactly match

Open lithomas1 opened this issue 1 year ago • 10 comments

lithomas1 avatar Jan 24 '24 23:01 lithomas1

Thanks for the PR but its intentional behaviour for skip/xfail'd tests to match multiple test cases, which is mostly useful for parametrized tests where adopters know they'll all fail.

honno avatar Jan 25 '24 11:01 honno

Thanks for the PR but its intentional behaviour for skip/xfail'd tests to match multiple test cases, which is mostly useful for parametrized tests where adopters know they'll all fail.

The problem, though is that it becomes impossible to skip some test cases without skipping other test cases that might not fail.

(e.g. skipping array_api_tests/test_array_object.py::test_setitem would also skip array_api_tests/test_array_object.py::test_setitem_masking )

lithomas1 avatar Jan 25 '24 13:01 lithomas1

I asked @lithomas1 to open this PR.

Can we make it so this either matches the full test with the parameterization, or a full test name without the parameterization part? Verboseness is not an issue for a file, but specificity is.

asmeurer avatar Jan 25 '24 18:01 asmeurer

Just to be sure, there are some warnings that are printed when an xfail doesn't match anything.

asmeurer avatar Jan 27 '24 02:01 asmeurer

I'll mull over but if you folks have an idea please update the PR. I don't think it's a good idea accepting this PR as-is because 1) it makes specifying parameterised tests very verbose 2) breaks existing configs without any warnings/etc.. I imagine some regex for pytest's square bracket syntax for atomic "parameter" test cases is what we'd want to look at, which would avoid the need to break existing configs too.

Maybe we can turn the lines of the skip file into a regex?

I think with a regex I can use $ to "end" the id I think. This works around my problem I think.

lithomas1 avatar Jan 27 '24 03:01 lithomas1

I technically don't need this PR anymore (since I resolved the failures mentioned above), but this might still be a good idea regardless.

(e.g. I'm pretty sure this also happens for torch in array-api-compat https://github.com/data-apis/array-api-compat/actions/runs/7590566384/job/20677289445#step:6:1588, where test_setitem is xfailed but test_setitem_masking xpasses)

lithomas1 avatar Jan 29 '24 01:01 lithomas1

Making it a regex would be even more breaking than this. The [] in any current pattern would be treated as a character class in a regex, and so all skip files would need to be updated. We could add something like a regex: prefix to make a line match as a regex.

However, I'm currently not convinced that this change is an issue. Are there xfails files out there that are making use of this prefix logic?

asmeurer avatar Jan 30 '24 20:01 asmeurer

Do we have reason to believe that it isn't enough to only accept

  • a test file (like array_api_tests/test_special_cases.py),
  • a test name (like array_api_tests/test_special_cases.py::test_binary), or
  • a full test with parameterization (like array_api_tests/test_special_cases.py::test_binary[__floordiv__(x1_i is +infinity and isfinite(x2_i) and x2_i > 0) -> +infinity])?

I don't see the value in accepting subsets other than those three cases, and trying to do so would risk lead to false positives like the test_setitem one.

The only thing I can think of is you might want to skip all parameterized tests for a given function like array_api_tests/test_special_cases.py::test_binary[__floordiv__], because maybe you know __floordiv__ can never work.

Another option would be to only match those three cases if the skip name starts with array_api_tests/, and do in matching only when it doesn't.

asmeurer avatar Feb 06 '24 21:02 asmeurer

Do we have reason to believe that it isn't enough to only accept

  • a test file (like array_api_tests/test_special_cases.py),
  • a test name (like array_api_tests/test_special_cases.py::test_binary), or
  • a full test with parameterization (like array_api_tests/test_special_cases.py::test_binary[__floordiv__(x1_i is +infinity and isfinite(x2_i) and x2_i > 0) -> +infinity])?

I don't see the value in accepting subsets other than those three cases, and trying to do so would risk lead to false positives like the test_setitem one.

I don't have a sense of usage actually tbf, so maybe no one's currently skipping any of the parametrized tests. I am generally worried that for things like the special case tests, if you want to skip/xfail a whole test case, you'd have to manually specify 100+ cases.

I think with a regex I can use $ to "end" the id I think. This works around my problem I think.

Making it a regex would be even more breaking than this. The [] in any current pattern would be treated as a character class in a regex, and so all skip files would need to be updated. We could add something like a regex: prefix to make a line match as a regex.

I agree regex for things like this seems a bit too much esp. considering how square brackets, but it is a well-defined and well-known format in the end if we did want to skip parameterized tests (or even specifically a subset of parameters) :thinking:

honno avatar Feb 28 '24 14:02 honno

Maybe we could also extend it to allow skipping test_binary[__floordiv__] or something like that. Or we could rewrite the tests so that __floordiv__ is part of the test name.

asmeurer avatar Mar 02 '24 01:03 asmeurer

We ran into the exact case of test_getitem and test_getitem_masking

Do we have reason to believe that it isn't enough to only accept

* a test file (like `array_api_tests/test_special_cases.py`),

* a test name (like `array_api_tests/test_special_cases.py::test_binary`), or

* a full test with parameterization (like `array_api_tests/test_special_cases.py::test_binary[__floordiv__(x1_i is +infinity and isfinite(x2_i) and x2_i > 0) -> +infinity]`)?

I'd also accept a directory/partial path, in case we split into directories later, but otherwise the list seems complete to me.

hameerabbasi avatar Jun 10 '24 12:06 hameerabbasi

I've opened #273 with my suggestions.

hameerabbasi avatar Jun 10 '24 12:06 hameerabbasi

This was superceeded by https://github.com/data-apis/array-api-tests/pull/273

asmeurer avatar Jun 25 '24 16:06 asmeurer