-
Notifications
You must be signed in to change notification settings - Fork 9
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
Clear out uses of addAction in tests/ #955
Comments
@bradybray please address these
|
@cz4rs please address these
|
@JacobDomagala please address these
|
https://darma-tasking.github.io/docs/html/scheduler.html may be a useful point of reference in the documentation |
@PhilMiller I'm not certain how to address the test_pending_send. It seems any implementation I attempt causes a segfault or a failure of test coverage. Should I file a pull-request with my current commits so they can be discussed there? |
…al comments in subsequent pull requests.
…ction-in-tests #955 clear out uses of addAction in tests
This has gone as far as it's going to right now, so I'm closing it. Thanks, all. |
What Needs to be Done?
We want to largely deprecate casual usage of
theTerm()->addAction
, in favor of higher-level routines instead. The dominant replacement for a lot of uses should probably bevt::runInEpochCollective()
, with the associated action simply sequenced afterward inline.There are 77 such calls in total. The ones in
tests/unit/termination
may be appropriate to leave as is.Jonathan and I have decided that we'd like to split up this effort among the new-comers, to provide a bit of an exercise, and spread out some experience with the commit, PR, and CI workflow.
https://github.com/DARMA-tasking/vt/pull/953/files#diff-3ab9ba90e26bdf35497656cb95df5b39 gives an illustration of the desired kind of change
I'll post comments with sub-assignments. Please each submit a separate PR, and assign it to me @PhilMiller for review.
Is your feature request related to a problem? Please describe.
Describe potential solution outcome
Describe alternatives you've considered
Additional context
The text was updated successfully, but these errors were encountered: