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

Removed failing test xinfo on clang 16 #2740

Merged
merged 23 commits into from
Nov 8, 2023
Merged

Conversation

spectre-ns
Copy link
Contributor

Checklist

  • [ x] The title and commit message(s) are descriptive.
  • [ x] Small commits made to fix your PR have been squashed to avoid history pollution.
  • [ x] Tests have been added for new features or bug fixes.
  • [ x] API of new functions and classes are documented.

Description

Following up on some previous work. I've removed clang 16 from the CI package as having failing tests all the time reduces the usefulness of the CI builds. Once XTensor irons out the bugs captured here: #2703 we can add it back.

@spectre-ns
Copy link
Contributor Author

@tdegeus can I get your opinion on this PR? We collaborated on the original CI setup (Thanks again). I'd like to narrow the CI configurations and then expand only once we get things passing.

@tdegeus
Copy link
Member

tdegeus commented Nov 8, 2023

Hi. Thanks! Shouldn't we better fix our issue?

@tdegeus
Copy link
Member

tdegeus commented Nov 8, 2023

In the worse case we could even disable that one test on clang16 with a comment/warning. It is a convenience function anyway, so it's not too bad if it fails

@spectre-ns
Copy link
Contributor Author

In the worse case we could even disable that one test on clang16 with a comment/warning. It is a convenience function anyway, so it's not too bad if it fails

@tdegeus I like this. I will revise. I agree fixing the issue would be best but until someone finds the bandwidth to look into it, I think it's best to exclude the failing test so we don't inadvertently introduce additional failures.

@tdegeus
Copy link
Member

tdegeus commented Nov 8, 2023

Great. We can just add a preprocessor statement and a reference to #2694

@spectre-ns
Copy link
Contributor Author

@tdegeus I cannot reproduce the failure on windows. It must be an issue with the pre-processor statement but using my windows clang set up it properly skips the test as expected. Do you see an immediate problem?

@spectre-ns
Copy link
Contributor Author

Looks like it's a different test failing on windows I'll have to find that later

test/test_xinfo.cpp Outdated Show resolved Hide resolved
@tdegeus
Copy link
Member

tdegeus commented Nov 8, 2023

Hi! Sorry, I thought that you gave up for today. Let's see what this gives!

@tdegeus
Copy link
Member

tdegeus commented Nov 8, 2023

It seems that you're proving why it's not good to work with failing CI ;)

@tdegeus
Copy link
Member

tdegeus commented Nov 8, 2023

Should we debug in a separate PR?

@tdegeus tdegeus changed the title Removed failing clang 16 steps from CI Removed failing test xinfo on clang 16 Nov 8, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@tdegeus tdegeus merged commit 82bf5cb into xtensor-stack:master Nov 8, 2023
11 of 12 checks passed
@spectre-ns
Copy link
Contributor Author

Cool now we only need to find one more test

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.

2 participants