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

apply clang-format to Tests #1456

Merged
merged 39 commits into from
Jul 25, 2024
Merged

Conversation

tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Jun 21, 2024

part of #1248

@tigercosmos tigercosmos requested a review from seladb as a code owner June 21, 2024 07:36
@tigercosmos tigercosmos changed the base branch from master to dev June 21, 2024 07:36
@tigercosmos tigercosmos force-pushed the clang-format0621 branch 2 times, most recently from 61c6d59 to fafe143 Compare June 21, 2024 07:41
@tigercosmos
Copy link
Collaborator Author

It's weird. I checked the modification of LoggerTests.cpp, and it should not cause any error.

TestLogger                         : FAILED (D:\a\PcapPlusPlus\PcapPlusPlus\Tests\Pcap++Test\Tests\LoggerTests.cpp:213). Assert EQUAL failed:
   Actual:   20
   Expected: 21

@egecetin egecetin added this to the Augest 2024 Release milestone Jun 21, 2024
@seladb
Copy link
Owner

seladb commented Jun 25, 2024

It's weird. I checked the modification of LoggerTests.cpp, and it should not cause any error.

TestLogger                         : FAILED (D:\a\PcapPlusPlus\PcapPlusPlus\Tests\Pcap++Test\Tests\LoggerTests.cpp:213). Assert EQUAL failed:
   Actual:   20
   Expected: 21

Does it work for you locally?

@tigercosmos
Copy link
Collaborator Author

It's weird. I checked the modification of LoggerTests.cpp, and it should not cause any error.

TestLogger                         : FAILED (D:\a\PcapPlusPlus\PcapPlusPlus\Tests\Pcap++Test\Tests\LoggerTests.cpp:213). Assert EQUAL failed:
   Actual:   20
   Expected: 21

Does it work for you locally?

No, it doesn't. It's weird, as the code shouldn't make any difference of the logic.


// clang-format off
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't know why, but seems the formatting of this part will break the tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What the diference between before and after clang-format ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

like this: #define<tab>SEPARATOR '\\'

@@ -6,343 +6,342 @@
#include <cstring>
#include <sstream>

PTF_TEST_CASE(Asn1DecodingTest)
PTF_TEST_CASE(Asn1DecodingTest){ // Context specific
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation formatting is all messed up here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

true, let me fix it.

@tigercosmos tigercosmos mentioned this pull request Jul 7, 2024
9 tasks
@tigercosmos
Copy link
Collaborator Author

@seladb ready for review.

@tigercosmos tigercosmos requested a review from Dimi1010 July 7, 2024 14:17
Copy link
Owner

@seladb seladb left a comment

Choose a reason for hiding this comment

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

I only reviewed 14 files, but wanted to post my comments so far

Tests/Packet++Test/Tests/IPv4Tests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/NflogTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/Asn1Tests.cpp Show resolved Hide resolved
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 77.02162% with 574 lines in your changes missing coverage. Please review.

Project coverage is 80.60%. Comparing base (add1746) to head (718d14c).

Files Patch % Lines
Tests/Packet++Test/Tests/LdapTests.cpp 53.57% 91 Missing ⚠️
Tests/Pcap++Test/main.cpp 18.42% 62 Missing ⚠️
Tests/Pcap++Test/Tests/IpMacTests.cpp 58.97% 48 Missing ⚠️
Tests/Packet++Test/Tests/PacketTests.cpp 56.96% 34 Missing ⚠️
Tests/Packet++Test/main.cpp 12.82% 33 Missing and 1 partial ⚠️
Tests/Packet++Test/Tests/HttpTests.cpp 74.74% 24 Missing and 1 partial ⚠️
Tests/Packet++Test/Tests/SipSdpTests.cpp 85.20% 25 Missing ⚠️
Tests/Packet++Test/Utils/TestUtils.cpp 52.17% 19 Missing and 3 partials ⚠️
Tests/Pcap++Test/Tests/FilterTests.cpp 48.57% 15 Missing and 3 partials ⚠️
Tests/Packet++Test/Tests/DnsTests.cpp 73.84% 16 Missing and 1 partial ⚠️
... and 37 more
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1456      +/-   ##
==========================================
- Coverage   82.09%   80.60%   -1.50%     
==========================================
  Files         273      273              
  Lines       43551    44647    +1096     
  Branches     9340     9305      -35     
==========================================
+ Hits        35755    35987     +232     
- Misses       7207     8069     +862     
- Partials      589      591       +2     
Flag Coverage Δ
fedora39 74.24% <88.82%> (+0.02%) ⬆️
macos-12 80.40% <93.31%> (+0.08%) ⬆️
macos-13 79.82% <92.44%> (+0.12%) ⬆️
macos-14 79.74% <92.44%> (+0.12%) ⬆️
mingw32 70.69% <ø> (-0.02%) ⬇️
mingw64 70.69% <ø> (-0.02%) ⬇️
npcap 83.92% <ø> (-0.04%) ⬇️
rhel94 73.95% <88.27%> (+0.03%) ⬆️
ubuntu2004 53.06% <29.40%> (-4.19%) ⬇️
ubuntu2004-zstd 53.19% <29.40%> (-4.19%) ⬇️
ubuntu2204 73.92% <88.26%> (+0.03%) ⬆️
ubuntu2204-icpx 55.88% <ø> (-0.01%) ⬇️
unittest 80.60% <77.02%> (-1.50%) ⬇️
windows-2019 83.96% <ø> (-0.02%) ⬇️
windows-2022 83.97% <ø> (-0.02%) ⬇️
winpcap 83.94% <ø> (ø)
xdp 48.81% <61.54%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Owner

@seladb seladb left a comment

Choose a reason for hiding this comment

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

I reviewed 36 / 68 files so far, sharing a few more comments

Tests/Packet++Test/Tests/DhcpV6Tests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/EthAndArpTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/FtpTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/FtpTests.cpp Show resolved Hide resolved
Tests/Pcap++Test/Common/GlobalTestArgs.h Show resolved Hide resolved
Tests/Packet++Test/Tests/IcmpV6Tests.cpp Show resolved Hide resolved
Tests/Packet++Test/Tests/LdapTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/LdapTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/LdapTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/LdapTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/LdapTests.cpp Outdated Show resolved Hide resolved
@tigercosmos tigercosmos requested a review from seladb July 22, 2024 03:13
@seladb
Copy link
Owner

seladb commented Jul 22, 2024

@tigercosmos CI fails after these changes, can you take a look?

@tigercosmos
Copy link
Collaborator Author

@seladb Fixed the test. It seems that includes reordering matters, so I still made a change in 344ff70.

Tests/Packet++Test/Tests/LdapTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/LdapTests.cpp Outdated Show resolved Hide resolved
Tests/Packet++Test/Tests/PacketUtilsTests.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@seladb seladb left a comment

Choose a reason for hiding this comment

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

@tigercosmos I finished reviewing all changes in this PR. Please find a few last comments (plus previous comments that weren't fixed yet). Once we fixed them we should be good to merge this PR

Tests/Pcap++Test/Tests/TcpReassemblyTests.cpp Outdated Show resolved Hide resolved
Tests/Pcap++Test/Tests/TcpReassemblyTests.cpp Outdated Show resolved Hide resolved
Tests/Pcap++Test/Tests/TcpReassemblyTests.cpp Outdated Show resolved Hide resolved
@tigercosmos tigercosmos requested a review from seladb July 24, 2024 09:50
@seladb
Copy link
Owner

seladb commented Jul 25, 2024

@tigercosmos I have no idea why Codecov includes the test in the test coverage report, I'm sure it wasn't like that before and I'm not sure what's changed 😕

@egecetin
Copy link
Collaborator

Not checked recent changes but it is possible to add ignore rule to codecov.yml https://docs.codecov.com/docs/ignoring-paths#:~:text=You%20can%20use%20the%20top,will%20be%20skipped%20during%20processing.

@tigercosmos tigercosmos merged commit 123994c into seladb:dev Jul 25, 2024
38 checks passed
@tigercosmos tigercosmos deleted the clang-format0621 branch July 25, 2024 13:28
@tigercosmos
Copy link
Collaborator Author

@seladb Thanks for your patient review!

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.

5 participants