955 clear out uses of addAction in tests #974

cz4rs · 2020-08-07T08:00:52Z

Fixes #955

codecov · 2020-08-07T13:02:20Z

Codecov Report

Merging #974 into develop will increase coverage by 0.01%.
The diff coverage is 95.36%.

@@             Coverage Diff             @@
##           develop     #974      +/-   ##
===========================================
+ Coverage    77.24%   77.26%   +0.01%     
===========================================
  Files          656      656              
  Lines        25052    25037      -15     
===========================================
- Hits         19352    19344       -8     
+ Misses        5700     5693       -7

Impacted Files	Coverage Δ
src/vt/scheduler/scheduler.h	`100.00% <ø> (ø)`
tests/unit/location/test_location_common.h	`0.00% <0.00%> (ø)`
tests/unit/objgroup/test_objgroup.cc	`99.24% <97.87%> (+0.02%)`	⬆️
tests/unit/group/test_group.cc	`100.00% <100.00%> (ø)`
tests/unit/location/test_hops.extended.cc	`91.13% <100.00%> (-1.17%)`	⬇️
tests/unit/memory/test_memory_lifetime.cc	`99.33% <100.00%> (ø)`
tests/unit/pipe/test_callback_bcast.cc	`100.00% <100.00%> (ø)`

lifflander

This is looking great to me

cz4rs · 2020-08-07T22:54:32Z

there's still a couple of files to be cleared, the rest is landing on Monday

tests/unit/memory/test_memory_active.cc

tests/unit/memory/test_memory_lifetime.cc

cz4rs · 2020-08-11T14:00:12Z

files cleared:

tests/unit/group/test_group.cc:4
tests/unit/location/test_hops.extended.cc:1
tests/unit/location/test_location_common.h:1 >> Codecov warning spotted, but verifyCacheConsistency function that was modified seems to be used in test_location.cc
tests/unit/memory/test_memory_active.cc:2 >> not applicable
tests/unit/memory/test_memory_lifetime.cc:14 >> only partially applicable
tests/unit/objgroup/test_objgroup.cc:3
tests/unit/pipe/test_callback_bcast.cc:6
tests/unit/pipe/test_callback_bcast_collection.extended.cc:3 >> not applicable (proxy.destroy(); used in addAction)

cz4rs · 2020-08-11T16:30:44Z

documentation improvements:

remove copydoc for vt::sched::Scheduler::scheduler (which results in random "Turn the scheduler")
note: it would be better to improve documentation of the method instead, maybe we can use the line above - "polls every component..."
remove duplication by omitting \brief (use copydetails instead of copydoc)

lifflander

This looks great now, I think it's ready to merge once CI passes

cz4rs · 2020-08-12T16:42:46Z

@lifflander

This looks great now, I think it's ready to merge once CI passes

Yep, I think it's good to go :)

Side note: you might have seen that already, TimeTrigger tests are still failing from time to time (clang-8, macosx, mpich build)

The following tests FAILED:
	354 - vt:TestTimeTrigger.test_time_trigger_2_proc_4 (Failed)

plus there's one (new?) warning generated from that test:

 [295/480] Building CXX object tests/CMakeFiles/test_time_trigger_extended.dir/unit/timetrigger/test_time_trigger.extended.cc.o
../../tests/unit/timetrigger/test_time_trigger.extended.cc:121:41: warning: lambda capture 'cur_time' is not used [-Wunused-lambda-capture]
      trigger_period[i], [&triggered,i,&cur_time]{
                                      ~~^~~~~~~~

lifflander · 2020-08-12T16:50:04Z

@lifflander

This looks great now, I think it's ready to merge once CI passes

Yep, I think it's good to go :)

Side note: you might have seen that already, TimeTrigger tests are still failing from time to time (clang-8, macosx, mpich build)
The following tests FAILED:
	354 - vt:TestTimeTrigger.test_time_trigger_2_proc_4 (Failed)
plus there's one (new?) warning generated from that test:
 [295/480] Building CXX object tests/CMakeFiles/test_time_trigger_extended.dir/unit/timetrigger/test_time_trigger.extended.cc.o
../../tests/unit/timetrigger/test_time_trigger.extended.cc:121:41: warning: lambda capture 'cur_time' is not used [-Wunused-lambda-capture]
      trigger_period[i], [&triggered,i,&cur_time]{
                                      ~~^~~~~~~~

Yes, I saw that it is still failing. Planning to fix

lifflander · 2020-08-12T23:07:46Z

@cz4rs Ok, can you rebase and then we can merge?

lifflander · 2020-08-13T05:05:03Z

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

Clones removed
==============
+ tests/unit/memory/test_memory_lifetime.cc  -3

See the complete overview on Codacy

cz4rs · 2020-08-13T05:32:46Z

@cz4rs Ok, can you rebase and then we can merge?

rebased with the latest changes from develop

PhilMiller · 2020-08-17T15:24:24Z

tests/unit/objgroup/test_objgroup.cc

+  runInEpochCollective([&]{
+    // self-send a message and then broadcast
+    proxy[my_node].send<MyMsg, &MyObjA::handler>();
+    proxy.broadcast<MyMsg, &MyObjA::handler>();


@lifflander I think this is a scalability problem for cases where vt is configured and compiled on a host with large numbers of cores - the message count will scale with the number of cores, which can make it take an arbitrarily long time on a very wide node

PhilMiller · 2020-08-18T19:10:02Z

tests/unit/location/test_location_common.h

@@ -151,18 +151,13 @@ void verifyCacheConsistency(
    // perform the checks only at the end of the epoch
    // to ensure that all entity messages have been
    // correctly delivered before.


@lifflander I think this change may have fundamentally shifted the meaning of this test. Please double check

This diff is really confusing to read. Had to go back and look.

This test is not testing the same thing at all. It's now checking the state before. I should have caught this.

I will revert and (try to) handle this correctly in #989

2d67222 added in #989

cz4rs force-pushed the 955-clear-out-uses-of-addAction-in-tests-part2 branch from 18bb1ba to aa8ea3d Compare August 7, 2020 12:18

lifflander approved these changes Aug 7, 2020

View reviewed changes

cz4rs force-pushed the 955-clear-out-uses-of-addAction-in-tests-part2 branch from aa8ea3d to c97fc6f Compare August 10, 2020 10:23

lifflander requested changes Aug 10, 2020

View reviewed changes

cz4rs force-pushed the 955-clear-out-uses-of-addAction-in-tests-part2 branch 2 times, most recently from cbd4f70 to c6e622d Compare August 11, 2020 12:19

cz4rs force-pushed the 955-clear-out-uses-of-addAction-in-tests-part2 branch 3 times, most recently from 6cede9d to 24a886c Compare August 11, 2020 16:25

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

cz4rs force-pushed the 955-clear-out-uses-of-addAction-in-tests-part2 branch 2 times, most recently from ecfd4aa to 334a7a3 Compare August 12, 2020 16:08

cz4rs marked this pull request as ready for review August 12, 2020 16:24

lifflander approved these changes Aug 12, 2020

View reviewed changes

cz4rs added 5 commits August 13, 2020 07:02

#955 use runInEpochCollective instead of addAction

9664b71

#955 remove addAction usage from test_callback_bcast.cc

07a8158

#955 remove addAction usage from test_memory_lifetime.cc

ed4e3eb

#955 improve Scheduler documentation

73d20fd

#955 remove addAction usage from test_objgroup.cc

4c45a7c

cz4rs force-pushed the 955-clear-out-uses-of-addAction-in-tests-part2 branch from c4bd1fc to 4c45a7c Compare August 13, 2020 05:03

lifflander merged commit 56c37d7 into develop Aug 13, 2020

cz4rs mentioned this pull request Aug 13, 2020

Clear out uses of addAction in tests/ #955

Closed

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

PhilMiller reviewed Aug 17, 2020

View reviewed changes

PhilMiller reviewed 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

cz4rs mentioned this pull request Aug 19, 2020

955 clear out uses of addAction from tests #989

Merged

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

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 #974

955 clear out uses of addAction in tests #974

cz4rs commented Aug 7, 2020

codecov bot commented Aug 7, 2020 •

edited

Loading

lifflander left a comment

cz4rs commented Aug 7, 2020

cz4rs commented Aug 11, 2020 •

edited

Loading

cz4rs commented Aug 11, 2020 •

edited

Loading

lifflander left a comment •

edited

Loading

cz4rs commented Aug 12, 2020

lifflander commented Aug 12, 2020

lifflander commented Aug 12, 2020

lifflander commented Aug 13, 2020

cz4rs commented Aug 13, 2020

PhilMiller Aug 17, 2020

PhilMiller Aug 18, 2020

lifflander Aug 18, 2020

cz4rs Aug 18, 2020

cz4rs Aug 19, 2020 •

edited

Loading

955 clear out uses of addAction in tests #974

955 clear out uses of addAction in tests #974

Conversation

cz4rs commented Aug 7, 2020

codecov bot commented Aug 7, 2020 • edited Loading

Codecov Report

lifflander left a comment

Choose a reason for hiding this comment

cz4rs commented Aug 7, 2020

cz4rs commented Aug 11, 2020 • edited Loading

cz4rs commented Aug 11, 2020 • edited Loading

lifflander left a comment • edited Loading

Choose a reason for hiding this comment

cz4rs commented Aug 12, 2020

lifflander commented Aug 12, 2020

lifflander commented Aug 12, 2020

lifflander commented Aug 13, 2020

cz4rs commented Aug 13, 2020

PhilMiller Aug 17, 2020

Choose a reason for hiding this comment

PhilMiller Aug 18, 2020

Choose a reason for hiding this comment

lifflander Aug 18, 2020

Choose a reason for hiding this comment

cz4rs Aug 18, 2020

Choose a reason for hiding this comment

cz4rs Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2020 •

edited

Loading

cz4rs commented Aug 11, 2020 •

edited

Loading

cz4rs commented Aug 11, 2020 •

edited

Loading

lifflander left a comment •

edited

Loading

cz4rs Aug 19, 2020 •

edited

Loading