955 Clear out uses of addAction in tests #979

JacobDomagala · 2020-08-11T13:22:05Z

Following tests are left unchanged (addAction() is a part of termination namespace so i thought its usage here makes sense)

tests/unit/termination/test_termination_action_callable.extended.cc:2
tests/unit/termination/test_termination_action_common.impl.h:2
tests/unit/termination/test_termination_reset.cc:2

codecov · 2020-08-11T14:02:01Z

Codecov Report

Merging #979 into develop will increase coverage by 0.04%.
The diff coverage is 96.90%.

@@             Coverage Diff             @@
##           develop     #979      +/-   ##
===========================================
+ Coverage    77.35%   77.40%   +0.04%     
===========================================
  Files          656      656              
  Lines        25110    25129      +19     
===========================================
+ Hits         19424    19451      +27     
+ Misses        5686     5678       -8

Impacted Files	Coverage Δ
.../unit/rdma/test_rdma_collection_handle.extended.cc	`4.58% <0.00%> (+0.16%)`	⬆️
...nit/sequencer/test_sequencer_extensive.extended.cc	`100.00% <ø> (ø)`
...s/unit/sequencer/test_sequencer_nested.extended.cc	`100.00% <ø> (ø)`
tests/unit/pipe/test_callback_func.cc	`96.15% <100.00%> (+0.15%)`	⬆️
tests/unit/pipe/test_callback_func_ctx.cc	`97.67% <100.00%> (-0.06%)`	⬇️
tests/unit/pipe/test_callback_send.cc	`100.00% <100.00%> (ø)`
...nit/pipe/test_callback_send_collection.extended.cc	`98.01% <100.00%> (+0.31%)`	⬆️
tests/unit/sequencer/test_sequencer.extended.cc	`100.00% <100.00%> (ø)`
...ests/unit/sequencer/test_sequencer_for.extended.cc	`100.00% <100.00%> (ø)`
...unit/sequencer/test_sequencer_parallel.extended.cc	`100.00% <100.00%> (ø)`
... and 3 more

tests/unit/pipe/test_callback_send_collection.extended.cc

tests/unit/rdma/test_rdma_collection_handle.extended.cc

tests/unit/sequencer/test_sequencer.extended.cc

PhilMiller · 2020-08-14T20:14:31Z

tests/unit/sequencer/test_sequencer_extensive.extended.cc

    }                                                                   \
-  } while (false);
+  } while (false);                                                      \

 #define FN_APPLY(SEQ_HAN, SEQ_FN, NODE, MSG_TYPE, ___, IS_TAG)          \


@lifflander
Just noticing that the FN_APPLY and CALL_EXPAND macros are each used only once below. That makes their definition as macros seem sort of silly

tests/unit/sequencer/test_sequencer_extensive.extended.cc

PhilMiller · 2020-08-14T20:25:34Z

For the tests you left alone, here are my thoughts:

tests/unit/termination/test_termination_reset.cc:2 - fine as is
tests/unit/termination/test_termination_action_callable.extended.cc:2 - it would be good to have runSchedulerThrough(epoch); EXPECT_EQ(num, 2); at the end of the test, though that maybe collides with the broader testing harness in use in channel and action
tests/unit/termination/test_termination_action_common.impl.h:2 - goes along with the previous case, and could maybe be restructured in whole.

PhilMiller · 2020-08-14T21:24:58Z

Note to self: the files addressed match the files assigned

lifflander

Overall, this looks good to me. I don't see any problems as long as the tests pass consistently,

tests/unit/pipe/test_callback_send_collection.extended.cc

tests/unit/sequencer/test_sequencer.extended.cc

tests/unit/sequencer/test_sequencer_extensive.extended.cc

lifflander · 2020-08-14T23:09:04Z

@JacobDomagala Can you address any comments for change and then rebase this on develop?

bradybray

@JacobDomagala Should line 91 of test_callback_func be changed from runInEpochCollective([=]{ to runInEpochCollective([&}{ so that it has the proper context passed in?

bradybray

Looks good. @JacobDomagala If the = does not need to be replaced would you explain why? AFAIK the & operator in runInEpochCollective is the pass the proper context to the function. This way it has access to the variables in the previous scope. Is this correct?

tests/unit/pipe/test_callback_func.cc

tests/unit/pipe/test_callback_func_ctx.cc

tests/unit/pipe/test_callback_send.cc

cz4rs

looks good overall to me
depending on the outcome of the discussion about proxy.destroy(); I might need to revisit test_callback_bcast_collection.extended.cc

tests/unit/pipe/test_callback_send_collection.extended.cc

lifflander · 2020-08-19T02:38:30Z

Looks good. @JacobDomagala If the = does not need to be replaced would you explain why? AFAIK the & operator in runInEpochCollective is the pass the proper context to the function. This way it has access to the variables in the previous scope. Is this correct?

@bradybray Both & and = will supply the context. The difference is whether it's passed by reference or value. = will call the copy constructor on all captured variables. & will just provide a reference to each captured variable. From reading the changed code, most of the callsites just a few small variables are captured (like NodeType which is a int16_t). In those cases, = is fine. The older interface vt::theTerm()->addAction often captures the lambda context by value because it doesn't execute right away. In that case, variables can go out of scope before it executes causing an ugly crash (reference to variable popped off the stack---i.e., junk)

lifflander · 2020-08-19T02:39:41Z

@JacobDomagala Is this ready to rebase and merge?

JacobDomagala · 2020-08-19T10:03:51Z

@JacobDomagala Is this ready to rebase and merge?

Yes, rebased on top of develop and it's ready to be merged.

lifflander · 2020-08-19T18:20:44Z

@JacobDomagala Github won't let you merge this because you haven't signed your commits with your GPG key. Did you set up GPG? You need to attach it to git so it adds your signature to the commits.

… applicable and fix lambda formatting

…CallbackSendCollection instead of checking them during Collection destruction

…r checking TestCallbackSendCollection test results

JacobDomagala · 2020-08-19T18:33:04Z

@JacobDomagala Github won't let you merge this because you haven't signed your commits with your GPG key. Did you set up GPG? You need to attach it to git so it adds your signature to the commits.

Yes, it seems that i lost my commits signatures after i rebased the branch. Should be ok now.

lifflander · 2020-08-19T18:34:07Z

Here is an overview of what got changed by this pull request:

Clones added
============
- tests/unit/pipe/test_callback_send_collection.extended.cc  2
- tests/unit/pipe/test_callback_func_ctx.cc  1
         

Clones removed
==============
+ tests/unit/pipe/test_callback_send.cc  -1

See the complete overview on Codacy

JacobDomagala force-pushed the 955-clear-out-uses-of-addAction-in-tests-part3 branch from c6842b0 to 12cf645 Compare August 11, 2020 13:26

JacobDomagala requested review from lifflander and PhilMiller August 11, 2020 13:27

JacobDomagala force-pushed the 955-clear-out-uses-of-addAction-in-tests-part3 branch from 12cf645 to a5cdad1 Compare August 11, 2020 13:43

JacobDomagala force-pushed the 955-clear-out-uses-of-addAction-in-tests-part3 branch 2 times, most recently from cb25503 to 52ece3c Compare August 11, 2020 15:55

lifflander added the 1.0.0-beta.10 label Aug 11, 2020

lifflander changed the title ~~#955 Clear out uses of addAction in tests~~ 955 Clear out uses of addAction in tests Aug 11, 2020

JacobDomagala force-pushed the 955-clear-out-uses-of-addAction-in-tests-part3 branch from 52ece3c to ae755fb Compare August 13, 2020 07:10

JacobDomagala marked this pull request as ready for review August 13, 2020 07:11

JacobDomagala requested a review from pnstickne August 13, 2020 19:02

PhilMiller reviewed Aug 14, 2020

View reviewed changes

tests/unit/pipe/test_callback_send_collection.extended.cc Outdated Show resolved Hide resolved

PhilMiller reviewed Aug 14, 2020

View reviewed changes

tests/unit/rdma/test_rdma_collection_handle.extended.cc Outdated Show resolved Hide resolved

PhilMiller reviewed Aug 14, 2020

View reviewed changes

tests/unit/sequencer/test_sequencer.extended.cc Show resolved Hide resolved

PhilMiller reviewed Aug 14, 2020

View reviewed changes

tests/unit/sequencer/test_sequencer_extensive.extended.cc Show resolved Hide resolved

PhilMiller requested review from bradybray and cz4rs August 14, 2020 21:25

lifflander approved these changes Aug 14, 2020

View reviewed changes

tests/unit/pipe/test_callback_send_collection.extended.cc Outdated Show resolved Hide resolved

tests/unit/sequencer/test_sequencer.extended.cc Show resolved Hide resolved

tests/unit/sequencer/test_sequencer_extensive.extended.cc Show resolved Hide resolved

JacobDomagala force-pushed the 955-clear-out-uses-of-addAction-in-tests-part3 branch from ae755fb to fe7f32b Compare August 16, 2020 17:24

bradybray reviewed Aug 16, 2020

View reviewed changes

tests/unit/pipe/test_callback_func.cc Outdated Show resolved Hide resolved

tests/unit/pipe/test_callback_func_ctx.cc Outdated Show resolved Hide resolved

tests/unit/pipe/test_callback_send.cc Outdated Show resolved Hide resolved

JacobDomagala force-pushed the 955-clear-out-uses-of-addAction-in-tests-part3 branch from 86b047c to e9dc906 Compare August 17, 2020 12:11

cz4rs approved these changes Aug 17, 2020

View reviewed changes

PhilMiller requested changes Aug 17, 2020

View reviewed changes

tests/unit/pipe/test_callback_send_collection.extended.cc Outdated Show resolved Hide resolved

JacobDomagala requested a review from PhilMiller August 18, 2020 09:31

PhilMiller requested changes Aug 18, 2020

View reviewed changes

tests/unit/pipe/test_callback_send_collection.extended.cc Outdated Show resolved Hide resolved

PhilMiller approved these changes Aug 18, 2020

View reviewed changes

PhilMiller mentioned this pull request Aug 18, 2020

Adding an action in the terminator should produce/consume on epoch? #649

Closed

JacobDomagala force-pushed the 955-clear-out-uses-of-addAction-in-tests-part3 branch from 374c982 to 8f1fc53 Compare August 19, 2020 08:40

JacobDomagala added 4 commits August 19, 2020 20:31

#955 Clear out uses of addAction in tests

7d80b9f

#955 Change to explicit lambda capture in 'runInEpochCollective' when…

9e9b3b1

… applicable and fix lambda formatting

#955 Use subsequent runInEpochCollective call to check result of Test…

3441de5

…CallbackSendCollection instead of checking them during Collection destruction

#955 Use collection broadcast message instead of sending callbacks fo…

fa70854

…r checking TestCallbackSendCollection test results

JacobDomagala force-pushed the 955-clear-out-uses-of-addAction-in-tests-part3 branch from 8f1fc53 to fa70854 Compare August 19, 2020 18:31

lifflander merged commit 0c5a46c into develop Aug 20, 2020

lifflander removed the 1.0.0-beta.10 label Aug 20, 2020

This was referenced Aug 25, 2020

TestCallbackSendCollection failure on some build configuration #1004

Closed

GroupCollective setup races with continuation registration code #1006

Closed

cz4rs mentioned this pull request Jun 15, 2021

move vt version number into a separate file #1467

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

955 Clear out uses of addAction in tests #979

955 Clear out uses of addAction in tests #979

JacobDomagala commented Aug 11, 2020 •

edited

Loading

codecov bot commented Aug 11, 2020 •

edited

Loading

PhilMiller Aug 14, 2020

PhilMiller commented Aug 14, 2020

PhilMiller commented Aug 14, 2020

lifflander left a comment

lifflander commented Aug 14, 2020

bradybray left a comment

bradybray left a comment

cz4rs left a comment

lifflander commented Aug 19, 2020 •

edited

Loading

lifflander commented Aug 19, 2020

JacobDomagala commented Aug 19, 2020

lifflander commented Aug 19, 2020

JacobDomagala commented Aug 19, 2020 •

edited

Loading

lifflander commented Aug 19, 2020

955 Clear out uses of addAction in tests #979

955 Clear out uses of addAction in tests #979

Conversation

JacobDomagala commented Aug 11, 2020 • edited Loading

codecov bot commented Aug 11, 2020 • edited Loading

Codecov Report

PhilMiller Aug 14, 2020

Choose a reason for hiding this comment

PhilMiller commented Aug 14, 2020

PhilMiller commented Aug 14, 2020

lifflander left a comment

Choose a reason for hiding this comment

lifflander commented Aug 14, 2020

bradybray left a comment

Choose a reason for hiding this comment

bradybray left a comment

Choose a reason for hiding this comment

cz4rs left a comment

Choose a reason for hiding this comment

lifflander commented Aug 19, 2020 • edited Loading

lifflander commented Aug 19, 2020

JacobDomagala commented Aug 19, 2020

lifflander commented Aug 19, 2020

JacobDomagala commented Aug 19, 2020 • edited Loading

lifflander commented Aug 19, 2020

JacobDomagala commented Aug 11, 2020 •

edited

Loading

codecov bot commented Aug 11, 2020 •

edited

Loading

lifflander commented Aug 19, 2020 •

edited

Loading

JacobDomagala commented Aug 19, 2020 •

edited

Loading