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.

Sign up for GitHub

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

Jump to bottom

test: Separate run_auto_tests into a library #1505

Closed
wants to merge 1 commit into from
Closed

test: Separate run_auto_tests into a library #1505

wants to merge 1 commit into from

Conversation

zugz
Copy link

@zugz zugz commented May 4, 2020 •
edited by iphydf
Loading

This change is Reviewable

@restyled-io restyled-io bot mentioned this pull request May 4, 2020
Closed
@iphydf iphydf changed the title Separate run_auto_tests into a library refactor: Separate run_auto_tests into a library May 5, 2020
@auto-add-label auto-add-label bot added the refactor Refactoring production code, eg. renaming a variable, not affecting semantics label May 5, 2020
@iphydf iphydf changed the title refactor: Separate run_auto_tests into a library test: Separate run_auto_tests into a library May 5, 2020
@auto-add-label auto-add-label bot added test Adding missing tests, refactoring tests; no production code change and removed refactor Refactoring production code, eg. renaming a variable, not affecting semantics labels May 5, 2020
@zugz zugz force-pushed the libAutoTest branch 3 times, most recently from f67e7e4 to 32c0d7a Compare May 5, 2020 22:29
iphydf
iphydf requested changes May 5, 2020
View reviewed changes
auto_tests/BUILD.bazel Outdated
@@ -8,8 +8,13 @@ cc_library(

cc_library(
name = "run_auto_test",
testonly = True,
srcs = ["run_auto_test.c"],
Copy link
Member

Choose a reason for hiding this comment

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

This should still be testonly. We don't want to link this into production code.

auto_tests/BUILD.bazel Outdated
@@ -8,8 +8,13 @@ cc_library(

cc_library(
name = "run_auto_test",
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename this to auto_test_support? Right now, all the "_test.c" files are actually tests. We use that fact to generate build/test rules for them. Either this, or add run_auto_test to an exclusion list below where we create those rules.

auto_tests/conference_test.c
}
}

for (uint16_t i = 0; i < NUM_GROUP_TOX; ++i) {
const uint32_t num_frozen = tox_conference_offline_peer_count(toxes[i], 0, nullptr);
const uint32_t num_frozen = tox_conference_offline_peer_count(autotoxes[i].tox, 0, nullptr);
ck_assert_msg(num_frozen <= max_frozen,
"tox #%u has too many offline peers: %u\n",
Copy link
Member

Choose a reason for hiding this comment

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

c-toxcore/auto_tests/conference_test.c:260: failed `num_frozen <= max_frozen': tox #3 has too many offline peers: 15

This started happening for me in this PR.

@pull-request-attention pull-request-attention bot assigned zugz and unassigned zugz May 5, 2020
@codecov
Copy link

codecov bot commented May 6, 2020 •
edited
Loading

Codecov Report

Merging #1505 (cc7e3d0) into master (00f2f41) will decrease coverage by 0.03%.
The diff coverage is 96.66%.

❗ Current head cc7e3d0 differs from pull request most recent head 35c279b. Consider uploading reports for the commit 35c279b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
- Coverage   83.95%   83.91%   -0.04%     
==========================================
  Files          87       87              
  Lines       16257    16270      +13     
==========================================
+ Hits        13648    13653       +5     
- Misses       2609     2617       +8
Impacted Files Coverage Δ
auto_tests/auto_test_support.c 89.43% <89.43%> (ø)
auto_tests/conference_av_test.c 96.70% <97.61%> (-0.06%) ⬇️
auto_tests/conference_double_invite_test.c 100.00% <100.00%> (ø)
auto_tests/conference_invite_merge_test.c 100.00% <100.00%> (+2.60%) ⬆️
auto_tests/conference_peer_nick_test.c 100.00% <100.00%> (ø)
auto_tests/conference_test.c 99.39% <100.00%> (-0.03%) ⬇️
auto_tests/friend_connection_test.c 100.00% <100.00%> (ø)
auto_tests/lossless_packet_test.c 100.00% <100.00%> (ø)
auto_tests/lossy_packet_test.c 100.00% <100.00%> (ø)
auto_tests/overflow_recvq_test.c 100.00% <100.00%> (ø)
... and 12 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 2db6599...35c279b. Read the comment docs.

@zugz zugz force-pushed the libAutoTest branch 2 times, most recently from 79ffdec to cc7e3d0 Compare May 8, 2020 23:22
iphydf
iphydf requested changes May 14, 2020
View reviewed changes
auto_tests/auto_test_support.c
tox_kill(autotoxes[i].tox);
}

if (autotoxes[i].state != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

if is not needed for free.

Copy link
Member

Choose a reason for hiding this comment

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

Same.

iphydf
iphydf requested changes May 29, 2020
View reviewed changes
auto_tests/auto_test_support.c
{
fprintf(stderr, "Saving #%u\n", autotox->index);

if (autotox->save_state != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

No need for if.

Copy link
Member

Choose a reason for hiding this comment

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

This is still true.

auto_tests/auto_test_support.c
free(autotoxes[i].state);
}

if (autotoxes[i].save_state != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member

Choose a reason for hiding this comment

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

Same.

@iphydf iphydf added this to the v0.2.x milestone Dec 6, 2021
iphydf
iphydf requested changes Dec 6, 2021
View reviewed changes
auto_tests/auto_test_support.c
{
fprintf(stderr, "Saving #%u\n", autotox->index);

if (autotox->save_state != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

This is still true.

auto_tests/auto_test_support.c
tox_kill(autotoxes[i].tox);
}

if (autotoxes[i].state != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

Same.

auto_tests/auto_test_support.c
free(autotoxes[i].state);
}

if (autotoxes[i].save_state != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@iphydf
Copy link
Member

iphydf commented Jan 29, 2022

Superseded by #1932.

@iphydf iphydf closed this Jan 29, 2022
@iphydf iphydf modified the milestones: v0.2.x, v0.2.14 Feb 4, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iphydf iphydf iphydf requested changes

Assignees

@zugz zugz

Labels
test Adding missing tests, refactoring tests; no production code change
Projects
None yet
Milestone
v0.2.14
Development

Successfully merging this pull request may close these issues.
2 participants
@zugz @iphydf