Skip to content

Port test_issue_1974 from SLI to Py #3439

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

JanVogelsang
Copy link
Contributor

There still is an issue with the Python-version, as it triggers the "assert_thread_parallel" assertion (line 207 in vp_manager.cpp). I am not sure why, @heplesser could you take a look at that?

@JanVogelsang JanVogelsang changed the title Ported issue 1974 slitest to python Port test_issue_1974 from SLI to Py Mar 24, 2025
@JanVogelsang JanVogelsang requested a review from heplesser March 27, 2025 16:28
@heplesser
Copy link
Contributor

@JanVogelsang This one fails currently on macOS, I believe, because we skip the test when Music is not present, which confuses the result collection process. I will need to think about the skipping logic for these MPI tests once again.

It also seems that this PR contains many more files than it should. It would be good to wait with this one until we have #3450 merged.

@JanVogelsang
Copy link
Contributor Author

Oh true, this PR now also contains changes that #3445 contains, because that fix was required for this test to work. So we have to wait until that one is merged. #3450 is not a blocker for this one. While they are related, fixing it is not required for this test to work.

@heplesser
Copy link
Contributor

@JanVogelsang Could you review #3467? Once that is in place it should be straightforward to implement music-skipping by decorator here.

@JanVogelsang
Copy link
Contributor Author

@heplesser I implemented the music-skipping decorator and added it to the isuee_1974 python test.

@JanVogelsang
Copy link
Contributor Author

Blocked by #3470

@heplesser
Copy link
Contributor

@JanVogelsang This looks in principle good to me now, but I would prefer to merge #3450 first so that the change set for this PR becomes smaller, reduced to just the port of the test and the missing-music decorator.

@heplesser
Copy link
Contributor

@JanVogelsang This looks in principle good to me now, but I would prefer to merge #3450 first so that the change set for this PR becomes smaller, reduced to just the port of the test and the missing-music decorator.

Sorry, I meant to merge #3445 first, not #3450.

@JanVogelsang
Copy link
Contributor Author

@heplesser Can you rerun the test-suite now that the CI issue is merged?

@JanVogelsang
Copy link
Contributor Author

@JanVogelsang This looks in principle good to me now, but I would prefer to merge #3450 first so that the change set for this PR becomes smaller, reduced to just the port of the test and the missing-music decorator.

Sorry, I meant to merge #3445 first, not #3450.

Yes, I agree. We just need Dennis to take a very quick look at #3445

@heplesser
Copy link
Contributor

@JanVogelsang If think you need to merge master into your branch for this PR so the Ubuntu-update in master takes effect here. That should automatically trigger the CI.

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