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

Use source-based code coverage #132

Merged
merged 9 commits into from
Jan 20, 2021
Merged

Conversation

Bathtor
Copy link
Contributor

@Bathtor Bathtor commented Jan 17, 2021

Please make sure these boxes are checked, before submitting a new PR.

  • You reference which issue is being closed in the PR text (if applicable)
  • You ran rustfmt on the code base before submitting (on latest nightly with rustfmt support)

Issues

Helps with #89, but doesn't really fix it. Should improve our ability to spot actual coverage deficits, however.

Breaking Changes

None; no code was touched.

Other Changes

  • Use the new source-based coverage mechanism, which allows us to run tests with panics, i.e. everything in the "ignored" set.
  • Separate out the workflow for code coverage. It takes a long time to run and is mostly unnecessary for iteration on PRs.
  • Code coverage will now only be run for pushes to master or when requesting a review on a PR (since changes to coverage may be information a reviewer might want to know about).

@Bathtor Bathtor requested a review from adamhass January 17, 2021 15:55
@Bathtor Bathtor requested review from adamhass and removed request for adamhass January 17, 2021 16:43
@codecov-io
Copy link

codecov-io commented Jan 17, 2021

Codecov Report

Merging #132 (7cab826) into master (84be104) will increase coverage by 2.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   67.80%   70.64%   +2.83%     
==========================================
  Files         101       90      -11     
  Lines       15433    15865     +432     
==========================================
+ Hits        10465    11208     +743     
+ Misses       4968     4657     -311     
Impacted Files Coverage Δ
core/src/dispatch/mod.rs 78.64% <ø> (+2.75%) ⬆️
core/tests/dispatch_integration_tests.rs 97.13% <ø> (+21.01%) ⬆️
docs/examples/src/bin/helloworld.rs 0.00% <0.00%> (-100.00%) ⬇️
experiments/datastructures/build.rs 0.00% <0.00%> (-100.00%) ⬇️
experiments/dynamic-benches/build.rs 0.00% <0.00%> (-100.00%) ⬇️
docs/examples/src/bin/unstable_counter.rs 0.00% <0.00%> (-100.00%) ⬇️
feature-tests/protobuf-test/src/main.rs 0.00% <0.00%> (-95.35%) ⬇️
docs/examples/src/bin/buncher_config.rs 0.00% <0.00%> (-94.88%) ⬇️
docs/examples/src/bin/buncher_adaptive.rs 0.00% <0.00%> (-94.45%) ⬇️
docs/examples/src/bin/buncher_regular.rs 0.00% <0.00%> (-93.94%) ⬇️
... and 93 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84be104...7cab826. Read the comment docs.

@Bathtor Bathtor requested review from adamhass and removed request for adamhass January 17, 2021 18:27
@Bathtor Bathtor requested review from adamhass and removed request for adamhass January 17, 2021 20:44
@Bathtor
Copy link
Contributor Author

Bathtor commented Jan 18, 2021

Argh, it keeps failing without giving me an error message. I think it's running out of disk space, but I can't be sure. Anyone got any ideas?

@adamhass
Copy link
Collaborator

Will it fail if a test case fails?

You can try removing my flaky annoying test case that I only added the ignore flag to. Either comment out the code or remove it entirely, I'll have to redo it somehow. I've got #130 open for it.

fn remote_delivery_overflow_network_thread_buffers() {

@Bathtor
Copy link
Contributor Author

Bathtor commented Jan 18, 2021

Will it fail if a test case fails?

Yes...but I don't think that's the issue. But I'm sufficiently out of ideas to try it anyway :D

@Bathtor Bathtor requested review from adamhass and removed request for adamhass January 18, 2021 09:27
@Bathtor
Copy link
Contributor Author

Bathtor commented Jan 18, 2021

Yes...but I don't think that's the issue. But I'm sufficiently out of ideas to try it anyway :D

Nope...wasn't it :(

@adamhass
Copy link
Collaborator

I kept the actions tab open while it was running and this is the tail end of the logging for the failing "Run tests" step:

[...]
 Doc-tests kompact

running 61 tests
test src/actors/mod.rs - actors::Actor (line 90) ... ok
test src/actors/mod.rs - actors::ActorRaw (line 41) ... ok
test src/actors/mod.rs - actors::NetworkActor (line 201) ... ok
test src/actors/refs.rs - actors::refs::ActorRef<Ask<Request, Response>>::ask (line 598) ... FAILED
test src/actors/refs.rs - actors::refs::ActorRef<M>::ask_with (line 490) ... FAILED
test src/actors/refs.rs - actors::refs::ActorRefStrong<Ask<Request, Response>>::ask (line 324) ... FAILED
test src/actors/refs.rs - actors::refs::ActorRefStrong<M>::ask_with (line 245) ... FAILED
test src/component/context.rs - component::context::ComponentContext<CD>::log (line 101) ... FAILED
test src/component/definition.rs - component::definition::ComponentDefinition::spawn_local (line 73) ... FAILED
test src/component/mod.rs - component::Handled::block_on (line 65) ... FAILED
test src/dispatch/mod.rs - dispatch::NetworkConfig (line 53) ... FAILED
test src/dispatch/mod.rs - dispatch::NetworkDispatcher::new (line 250) ... FAILED
test src/lib.rs - (line 7) ... FAILED
test src/messaging/deser_macro.rs - match_deser (line 10) ... FAILED
test src/messaging/deser_macro.rs - match_deser (line 34) ... FAILED
test src/component/context.rs - component::context::ComponentContext<CD>::config (line 140) ... FAILED
test src/messaging/net_message.rs - messaging::net_message::NetData::try_deserialise_unchecked (line 400) ... FAILED
test src/messaging/net_message.rs - messaging::net_message::NetData::try_deserialise (line 351) ... FAILED
test src/messaging/net_message.rs - messaging::net_message::NetMessage::try_deserialise (line 209) ... FAILED
test src/messaging/net_message.rs - messaging::net_message::NetMessage::try_into_deserialised (line 146) ... FAILED
test src/ports.rs - ports::Port (line 24) ... FAILED
test src/ports.rs - ports::ProvidedPort (line 99) ... FAILED
test src/ports.rs - ports::RequiredPort (line 235) ... FAILED
test src/runtime/config.rs - runtime::config::KompactConfig (line 20) ... FAILED
test src/messaging/net_message.rs - messaging::net_message::NetMessage::try_deserialise_unchecked (line 263) ... FAILED
test src/runtime/config.rs - runtime::config::KompactConfig::build (line 350) ... FAILED
test src/runtime/config.rs - runtime::config::KompactConfig::system_components (line 192) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem (line 47) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::create (line 191) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::create_and_register (line 450) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::kill (line 760) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::kill_notify (line 795) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::config (line 154) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::register (line 421) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::register_by_alias (line 496) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::set_routing_policy (line 588) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::logger (line 136) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::shutdown_async (line 913) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::start (line 632) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::shutdown (line 891) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::start_notify (line 658) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::stop (line 691) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::stop_notify (line 724) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::create (line 1060) ... FAILED
test src/runtime/system.rs - runtime::system::KompactSystem::update_alias_registration (line 541) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::kill (line 1397) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::create_and_register (line 1133) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::kill_notify (line 1429) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::register (line 1109) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::register_by_alias (line 1169) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::set_routing_policy (line 1251) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::start (line 1291) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::shutdown_async (line 1465) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::start_notify (line 1314) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::stop_notify (line 1369) ... FAILED
test src/runtime/system.rs - runtime::system::SystemHandle::update_alias_registration (line 1209) ... FAILED
test src/timer/timer_manager.rs - timer::timer_manager::Timer::schedule_once (line 52) ... FAILED
test src/timer/timer_manager.rs - timer::timer_manager::Timer::schedule_periodic (line 102) ... FAILED
test src/utils/ask.rs - utils::ask::Ask<Request, Response>::complete_with (line 80) ... FAILED

But more importantly it fails after 59m and 59s which is probably not a coincidence?

@Bathtor
Copy link
Contributor Author

Bathtor commented Jan 18, 2021

But more importantly it fails after 59m and 59s which is probably not a coincidence?

Coincidence...hm, not sure. Yes it always fails pretty much there. But we shouldn't be hitting the time limit. It's 6h per job, not 1h, according to the docs.

It might simply be that this is the time it takes to fill some other quota.
It got the file size limit reached thingy once two runs ago (you can see it in the history), but I haven't gotten it since then.

But I still don't know what causes the issue. I'm wondering if we should file a bug report for GHA. There just isn't enough output to figure out what's going wrong and the logs bloody disappear after every run -.-

@adamhass
Copy link
Collaborator

Okay it's definitely the disk space usage. The logs fail to upload because of it. Did we try max_level_error for all the test runs?

actions/runner-images#2475.

@Bathtor Bathtor requested review from adamhass and removed request for adamhass January 20, 2021 08:53
@Bathtor
Copy link
Contributor Author

Bathtor commented Jan 20, 2021

Okay it's definitely the disk space usage. The logs fail to upload because of it. Did we try max_level_error for all the test runs?

I'm trying that now. But I guess the actual space hogger is the profiling data, which we, of course, don't want to reduce :D

@Bathtor
Copy link
Contributor Author

Bathtor commented Jan 20, 2021

Yeah, it didn't help...I guess if the GHA runners don't have enough space to store our profiling information, we just can't do this right now.

The only way to fix it would be to have it run our own runner, where we control amount of disk space. @segeljakt Have you done this by now for your CI? If yes, would be feasible to run just our code-coverage there as well?

@segeljakt
Copy link
Contributor

@Bathtor The runner is in a docker container so I can setup a second container for your runner if you would like. I am not sure how you want it configured, do you want some software to be pre-installed or does everything come with the GitHub actions YAML file?

@Bathtor
Copy link
Contributor Author

Bathtor commented Jan 20, 2021

@Bathtor The runner is in a docker container so I can setup a second container for your runner if you would like. I am not sure how you want it configured, do you want some software to be pre-installed or does everything come with the GitHub actions YAML file?

That would be great!
I think everything should come with the YAML file. Can you coordinate with @adamhass to get this set up, please?

Run Codecov job on self-hosted
@adamhass adamhass requested a review from segeljakt January 20, 2021 13:35
@segeljakt segeljakt requested review from segeljakt and removed request for segeljakt January 20, 2021 13:38
@adamhass adamhass requested review from adamhass and removed request for adamhass January 20, 2021 13:55
@adamhass adamhass requested review from adamhass and removed request for adamhass January 20, 2021 14:02
@adamhass
Copy link
Collaborator

Okay everything is good to me like this. The test-coverage is triggered on PR's the same way the rest of the CI is. I've commented out one test case that was dependent on the host system not running the tests with sudo privilege (which I've found unsound for a long time anyway).

The Codecov comment above is a bit weird due to it being edited by codecov rather than it posting a new comment. I think it will be better with future PR's.

If you're satisfied with the CI now Lars we can squash and merge this?

pull_request:
branches:
- master
types: [opened, reopened, synchronize]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we really want this to run with every single commit added to a PR? It's a 1h test suite. It'll take up a lot of resources on Klas' node.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It takes 13min on Klas machine 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whooot?!?

.shutdown()
.expect("Kompact didn't shut down properly");
}
// #[test]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to merge it with this test commented out? I kinda just did that as testy thingy to see if it helped to run it on GHA...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah it's too flaky. I'm gonna replace it with a better test with my next PR.

@adamhass adamhass merged commit 952c0ed into kompics:master Jan 20, 2021
@Bathtor Bathtor deleted the source-coverage branch October 18, 2021 21:39
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.

4 participants