Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test[cartesian]: conditional xfail instead of two tests #1885

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Feb 25, 2025

Description

Follow-up from PR #1882 to merge two test cases which only differed in the expected outcome. With this PR, we'll have one single test case with a conditional xfail as @havogt proposed in the GridTools slack channel.

Related issue: #1881

Requirements

  • All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the appropriate ADR inside the docs/development/ADRs/ folder.
    N/A

@romanc

This comment was marked as outdated.

@romanc romanc changed the title test[cartesian]: index cast: conditional xfail for dace backends ci: be strict about xfail Feb 25, 2025
@romanc
Copy link
Contributor Author

romanc commented Feb 25, 2025

Sorry for the changes after review request. I just realized that pytest will - by default - not complain very loudly if xfails unexpectedly pass. I changed the setting now to be strict and fail the testsuite if a test marked with xfail succeeds.

Copy link
Contributor

@twicki twicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic looks good!

@romanc

This comment was marked as off-topic.

@romanc romanc force-pushed the romanc/xfail-test-cleanup branch from 1a161dc to de4f84f Compare February 25, 2025 16:08
@romanc

This comment was marked as off-topic.

Follow-up from PR GridTools#1882 to merge
test cases, which only differ in their expected outcome. With this PR,
we'll have a single test case with conditional xfail instead.
@romanc romanc force-pushed the romanc/xfail-test-cleanup branch from de4f84f to 31d50d3 Compare February 26, 2025 09:41
@romanc romanc changed the title ci: be strict about xfail test[cartesian]: conditional xfail instead of two tests Feb 26, 2025
@romanc
Copy link
Contributor Author

romanc commented Feb 26, 2025

Sorry for the back and forth. I was starting to mix things in this PR. Let's use this PR strictly for merging the two test cases and then talk in PR #1888 about enabling xfail_strict by default.

@romanc romanc mentioned this pull request Feb 26, 2025
1 task
@FlorianDeconinck
Copy link
Contributor

cscs-ci run

@romanc
Copy link
Contributor Author

romanc commented Feb 27, 2025

Can someone with write access to give the green light here? Thanks 🙏

@havogt havogt merged commit 847d8ab into GridTools:main Feb 27, 2025
23 checks passed
@romanc romanc deleted the romanc/xfail-test-cleanup branch February 27, 2025 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants