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

add tests exposing co_broadcast and co_reduce bugs #72

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

everythingfunctional
Copy link
Member

To demonstrate the presence of the bugs you can run the isolated tests with either:

GASNET_PSHM_NODES=2 ./build/run-fpm.sh test -- -f '(caffeinate|co_broadcast)' -f '(caffeinate|derived type)' -d

or

GASNET_PSHM_NODES=2 ./build/run-fpm.sh test -- -f '(caffeinate|co_reduce)' -f '(caffeinate|derived type)' -d

@bonachea
Copy link
Member

bonachea commented Mar 7, 2024

Is there an open issue for these known defects?

If not, please open one with at least a minimal description of the failure.

On a related note, does our CI infrastructure have a way to mark "known failures"? If not, won't merging this result in CI failure for even unrelated PRs?

We should probably discuss our policy for how to handle known defects: specifically whether tests revealing a known failure should be committed pre-emptively with some kind of known failure indicator, or whether known defect reproducers should remain only in the issue tracker until there is a PR that repairs the defect and also adds a regression test for the fixed defect.

Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

Not convinced this should be merged as-is if it results in breaking CI for all PRs

@everythingfunctional
Copy link
Member Author

Yeah, totally wasn't expecting to merge this until we had a fix for each, but wanted to have concrete examples for discussion. I'll open an issue as well.

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