test: add tests for the ToxAV multithreading modes #1847

sudden6 · 2022-01-04T20:09:33Z

This tests tox_iterate in a separate thread, and the two ToxAV modes.

This change is

iphydf · 2022-01-04T21:24:06Z

auto_tests/toxav_mt_test.cc

+
+  auto poll_state = [](AV_State &av, uint32_t expected, uint32_t max_tries,
+                       uint32_t delay_ms) -> bool {
+    uint32_t i;


Unfortunately, this is C++, and cimple doesn't understand C++, so here comes the manual diagnostic:

toxav_mt_test.cc:408: variable `i' can be reduced in scope toxav_mt_test.cc:410: possibly to here

iphydf · 2022-01-04T21:30:50Z

auto_tests/toxav_mt_test.cc

+  // temporarily add bootstrap node to end of toxes, so we can iterate all
+  toxes.push_back(std::move(bootstrap));
+
+  uint32_t bootstrap_iteration = 0;


I'd move this down to just before the for-loop, and move the initialisation into the for-init-stmt.

CLAassistant · 2022-01-05T09:58:09Z

All committers have signed the CLA.

nurupo · 2022-01-05T10:44:02Z

Huh, me pushing a commit broke Github Actions on this PR. @sudden6 try removing my commit from the PR and then adding it back and pushing it from your account, maybe that will fix it. Keep the commit as it is, such that it's a1247a7 hash, just push it from your account instead. For the context, GitHub has disabled Actions on my account for some reason.

auto_tests/toxav_mt_test.cc

iphydf · 2022-01-05T18:52:33Z

auto_tests/toxav_mt_test.cc

+  ToxAV_Ptr toxAV_;
+
+  std::atomic_bool stop_threads;
+  std::atomic_bool incomming;


iphydf · 2022-01-05T18:53:07Z

auto_tests/toxav_mt_test.cc

+
+  std::atomic_bool video_received;
+  std::atomic_bool audio_received;
+  std::string name_;


What's the convention that makes name into name_ but leaves audio_received without trailing _?

unified it, all members now are snek_ case

iphydf · 2022-01-05T18:53:54Z

auto_tests/toxav_mt_test.cc

+
+}  // namespace
+
+static void test_av(bool combined) {


Why is this outside the namespace{}?

Moved inside

iphydf · 2022-01-05T18:58:18Z

auto_tests/toxav_mt_test.cc

+  std::mutex tox_loop_lock;
+
+  Tox_Ptr tox_;
+  bool combined_;


combined_av is a little clearer on what is being combined here.

iphydf · 2022-01-05T19:03:35Z

auto_tests/toxav_mt_test.cc

+
+// Make lines shorter
+using Clock = std::chrono::high_resolution_clock;
+using TimePoint = std::chrono::time_point<Clock>;


The naming convention in this file is a bit all over the place. E.g. TimePoint and Tox_Ptr. Also, some top level functions are snake_case, others are dromedaryCase. Since you're not using the Tox naming conventions (which are somewhat self-described by all the other sources), can you briefly describe them at the top of the file? Just a list of "kind of identifier: naming convention".

Should be mirroring random_testing.cc now

iphydf · 2022-01-05T19:04:24Z

auto_tests/toxav_mt_test.cc

+
+static void t_accept_friend_request_cb(Tox *m, const uint8_t *public_key, const uint8_t *data,
+                                       size_t length, void *userdata) {
+  if (length == 7 && std::memcmp("gentoo", data, 7) == 0) {


Do we expect other friend requests to arrive? Maybe we should assert that we don't.

Should be fixed now

iphydf · 2022-01-05T19:04:53Z

auto_tests/toxav_mt_test.cc

+static void t_accept_friend_request_cb(Tox *m, const uint8_t *public_key, const uint8_t *data,
+                                       size_t length, void *userdata) {
+  if (length == 7 && std::memcmp("gentoo", data, 7) == 0) {
+    ck_assert(tox_friend_add_norequest(m, public_key, nullptr) != (uint32_t)-1);


uint32_t(-1) or static_cast (I think I'd prefer the former, but the latter might be slightly clearer).

Fixed and opened #1855 because I copied this.

iphydf · 2022-01-09T00:18:45Z

CMakeLists.txt

  endif()
 endfunction()

+function(auto_test_cc target)


Maybe we can pass the suffix and avoid duplicating this? It's literally 1 byte difference between the two macros.

Does this work for you?

This tests tox_iterate in a separate thread, and the two ToxAV modes.

codecov · 2024-11-06T15:56:15Z

Codecov Report

❌ Patch coverage is 92.99065% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.63%. Comparing base (d23222c) to head (ebbe631).
⚠️ Report is 706 commits behind head on master.

Files with missing lines	Patch %	Lines
auto_tests/toxav_mt_test.cc	93.20%	14 Missing ⚠️
toxav/toxav.c	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1847      +/-   ##
==========================================
+ Coverage   81.11%   81.63%   +0.51%     
==========================================
  Files          81       82       +1     
  Lines       16052    16236     +184     
==========================================
+ Hits        13021    13254     +233     
+ Misses       3031     2982      -49

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

restyled-io bot mentioned this pull request Jan 4, 2022

Restyle Test/split iterate #1848

Closed

sudden6 changed the title ~~Test/split iterate~~ test: add tests for the ToxAV multithreading modes Jan 4, 2022

auto-add-label bot added the test Adding missing tests, refactoring tests; no production code change label Jan 4, 2022

sudden6 force-pushed the test/split_iterate branch 4 times, most recently from 649a24e to 08fc8f2 Compare January 4, 2022 21:01

iphydf requested changes Jan 4, 2022

View reviewed changes

pull-request-attention bot assigned sudden6 Jan 4, 2022

iphydf requested changes Jan 4, 2022

View reviewed changes

iphydf force-pushed the test/split_iterate branch 2 times, most recently from b977971 to 77fb904 Compare January 5, 2022 14:38

iphydf requested changes Jan 5, 2022

View reviewed changes

pull-request-attention bot assigned iphydf and unassigned sudden6 Jan 8, 2022

sudden6 force-pushed the test/split_iterate branch 3 times, most recently from dd0dd4e to 4cf603c Compare January 8, 2022 22:48

restyled-io bot mentioned this pull request Jan 8, 2022

Restyle test: add tests for the ToxAV multithreading modes #1858

Closed

iphydf reviewed Jan 9, 2022

View reviewed changes

pull-request-attention bot assigned sudden6 and unassigned iphydf Jan 9, 2022

sudden6 force-pushed the test/split_iterate branch 2 times, most recently from 0cc8c8f to 4cbb508 Compare January 9, 2022 10:05

restyled-io bot mentioned this pull request Jan 9, 2022

Restyle test: add tests for the ToxAV multithreading modes #1859

Closed

sudden6 force-pushed the test/split_iterate branch from fee16c8 to eae5537 Compare January 9, 2022 11:35

pull-request-attention bot assigned iphydf and unassigned sudden6 Jan 9, 2022

iphydf force-pushed the test/split_iterate branch from faa0eb2 to cb57cf7 Compare January 10, 2022 11:29

sudden6 requested a review from a team as a code owner January 10, 2022 11:29

iphydf force-pushed the test/split_iterate branch 2 times, most recently from 4a1e2d8 to 0b604ce Compare January 12, 2022 21:32

test: add test for multithreaded ToxAV

ebbe631

This tests tox_iterate in a separate thread, and the two ToxAV modes.

iphydf force-pushed the test/split_iterate branch from 0b604ce to ebbe631 Compare January 14, 2022 12:46

iphydf added this to the v0.2.x milestone Feb 4, 2022

iphydf added P3 Low priority and removed P3 Low priority labels Feb 5, 2022

iphydf modified the milestones: v0.2.x, v0.2.15 Feb 5, 2022

robinlinden modified the milestones: v0.2.15, v0.2.16 Feb 6, 2022

iphydf modified the milestones: v0.2.16, v0.2.17 Feb 6, 2022

iphydf modified the milestones: v0.2.17, v0.2.18, v0.2.19 Feb 23, 2022

iphydf modified the milestones: v0.2.19, v0.2.20 Apr 18, 2022

iphydf modified the milestones: v0.2.20, v0.2.21 Nov 6, 2024

iphydf modified the milestones: v0.2.21, v0.2.22 Jan 14, 2025

Green-Sky modified the milestones: v0.2.22, v0.2.23 Oct 1, 2025

test: add tests for the ToxAV multithreading modes #1847

Are you sure you want to change the base?

test: add tests for the ToxAV multithreading modes #1847

Uh oh!

Conversation

sudden6 commented Jan 4, 2022 • edited by iphydf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nurupo commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

sudden6 commented Jan 4, 2022 •

edited by iphydf

Loading

CLAassistant commented Jan 5, 2022 •

edited

Loading

nurupo commented Jan 5, 2022 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading