Fix frame header CC tests #1550

lpawelcz · 2024-08-19T14:51:37Z

This PR fixes the handling of absl:Span with test data input vectors for the frame_header tests.
It looks like we must copy the vectors over to the ZSTDFrameHeader struct and keep those there for the time of the test case execution and use absl::Span based on those copies.
Without this modification we observed that few of the test cases will always fail due to incorrect data under the input_buffer span. The interesting thing is that when we commented out all test cases except one of the failing ones (e.g. FrameHeaderTest.Success) we noticed that this particular test case started passing indicating that the issue can be triggered by execution of multiple test cases at the same time.

This PR also fixes some formatting errors that were picked up with clang-format version 18.1.8

Signed-off-by: Pawel Czarnecki <[email protected]>

cdleary · 2024-08-19T15:10:06Z

xls/modules/zstd/frame_header_test.cc

  ZSTD_frameHeader kHeader;
  size_t kResult;
+
+  absl::Span<const uint8_t> kBuffer() const {


google style is just buffer() since the member is buffer_

Changed from kBuffer() to buffer()

cdleary · 2024-08-19T15:12:38Z

xls/modules/zstd/frame_header_test.cc

+    return absl::MakeConstSpan(buffer_);
+  }
+
+  ZstdFrameHeader(absl::Span<const uint8_t> buffer, ZSTD_frameHeader& h,


google style says since it has a constructor it should be a class and not a struct

also passing mutable references is less preferrable here than passing by value -- for any large type (which the frameHeader may be) you can std::move the value

Changed ZstdFrameHeader to class and dropped passing by reference in favor of passing by value + std::move

cdleary · 2024-08-19T15:12:59Z

xls/modules/zstd/frame_header_test.cc

@@ -113,7 +126,7 @@ class FrameHeaderTest : public xls::IrTestBase {
  // expected values.
  void RunAndExpectFrameHeader(const ZstdFrameHeader& zstd_frame_header) {
    // Extend buffer contents to 128 bits if necessary.
-    const absl::Span<const uint8_t>& buffer = zstd_frame_header.kBuffer;
+    const absl::Span<const uint8_t>& buffer = zstd_frame_header.kBuffer();


Don't take refs to spans, spans are easily copied so we pass them and hold them by value

hzeller · 2024-08-19T17:49:14Z

xls/modules/zstd/BUILD

@@ -213,8 +213,6 @@ cc_test(
    tags = ["manual"],


Is there a reason why this test is marked "manual" ? Given that this was hiding an issue, it is probably good to have that enabled.
(Only if it is taking too many resources I'd consider it tagging manual. But then with a comment as explanation)

We marked all DSLX and C++ tests as manual so that those would be called only in the separate ZSTD-specific CI workflow. It explicitly calls all of those rules tagged as manual and the workflow is triggered when there is a PR that changes the xls/modules/zstd directory or there is a push to main branch that changes it.

This was first requested in #1315 (comment).
After that we also decided to tag some of the build rules as manual because e.g. place_and_route rule is time consuming and it makes sense to not run it in every CI run (see #1315 (comment)).

@proppy do the reasons for manually running tests still hold ? I think it is more a danger to not run tests all the time, because then hidden issues will go undetected.

@hzeller the initial reason was that there were wip tests capturing current limitations of the implementation, but I don't believe that's the case anymore is it @lpawelcz ?

@proppy Note you can disable them in gtest by calling them TEST(MyTest, DISABLED_DoStuffThatOnlyCompilesButFailAtRuntime) { ... }

https://google.github.io/googletest/advanced.html#temporarily-disabling-tests

@hzeller the initial reason was that there were wip tests capturing current limitations of the implementation, but I don't believe that's the case anymore is it @lpawelcz ?

Yes, that was the initial reason that no longer holds.
There is also a second reason which is the length of the CI workflow. We wanted to avoid running all of our tests each time because both the C++ tests and the benchmarks (build rules for synthesis, place and route; run rules for IR benchmarks) are very time consuming.

With the suggestion from @cdleary I removed the manual tags for the C++ tests so that at least those will be executed in the main CI workflow.

hzeller · 2024-08-19T17:58:11Z

Can you also run

bant dwyu //xls/modules/...

? Looks like there is an unnecessary @com_google_absl//absl/status dependency in //xls/modules/zstd:data_generator

lpawelcz · 2024-08-20T08:07:20Z

Can you also run
bant dwyu //xls/modules/...
? Looks like there is an unnecessary @com_google_absl//absl/status dependency in //xls/modules/zstd:data_generator

I checked the code with bant (current main at hzeller/bant@d033e1a) and it doesn't report anything.

cdleary

Looks good with the member changes.

Would be good to not mark the c++ tests as manual just the backend targets if possible.

cdleary · 2024-08-20T14:29:54Z

xls/modules/zstd/frame_header_test.cc

-struct ZstdFrameHeader {
-  absl::Span<const uint8_t> kBuffer;
+class ZstdFrameHeader {
+ public:
  ZSTD_frameHeader kHeader;
  size_t kResult;


These members should be private with accessors and shouldn't be named kSomething because they're not constexpr.

hzeller · 2024-08-20T17:45:47Z

I checked the code with bant (current main at hzeller/bant@d033e1a) and it doesn't report anything.

@lpawelcz

Interesting. Since off-topic here, can you open a bug in bant, reference this PR and show what you see as output of

# -vvv for extra verbose explaining what it is doing internally
bant dwyu -vvv //xls/modules/zstd:data_generator

I do get a

buildozer 'remove deps @com_google_absl//absl/status' //xls/modules/zstd:data_generator

because there is no absl/status/status.h included (grep status.h xls/modules/zstd/data_generator.{h,cc}) in any of the source files.

Internal-tag: [#64294] Signed-off-by: Pawel Czarnecki <[email protected]>

lpawelcz · 2024-08-21T09:50:37Z

I checked the code with bant (current main at hzeller/bant@d033e1a) and it doesn't report anything.

@lpawelcz

Interesting. Since off-topic here, can you open a bug in bant, reference this PR and show what you see as output of
# -vvv for extra verbose explaining what it is doing internally
bant dwyu -vvv //xls/modules/zstd:data_generator
I do get a
buildozer 'remove deps @com_google_absl//absl/status' //xls/modules/zstd:data_generator
because there is no absl/status/status.h included (grep status.h xls/modules/zstd/data_generator.{h,cc}) in any of the source files.

This was error on my side when using bant. Now I was able to get the correct output. I removed the unnecessary dependency from the BUILD file.

lpawelcz added 2 commits August 19, 2024 16:10

modules/zstd: fix formatting errors in C++ files

88beb56

Signed-off-by: Pawel Czarnecki <[email protected]>

modules/zstd: fix formatting errors in BUILD file

94ebb25

Signed-off-by: Pawel Czarnecki <[email protected]>

cdleary requested changes Aug 19, 2024

View reviewed changes

hzeller reviewed Aug 19, 2024

View reviewed changes

lpawelcz force-pushed the 64294-fix-frame-header-tests branch from f404d7d to 63d72db Compare August 20, 2024 08:06

cdleary approved these changes Aug 20, 2024

View reviewed changes

lpawelcz added 2 commits August 21, 2024 09:51

modules/zstd/frame_header_test: Fix absl::Span handling

99af284

Internal-tag: [#64294] Signed-off-by: Pawel Czarnecki <[email protected]>

modules/zstd: remove 'manual' tag from C++ test rules

1671da5

Internal-tag: [#64294] Signed-off-by: Pawel Czarnecki <[email protected]>

lpawelcz mentioned this pull request Aug 21, 2024

Bant won't detect dependency that should be removed hzeller/bant#14

Closed

lpawelcz force-pushed the 64294-fix-frame-header-tests branch from 63d72db to 1671da5 Compare August 21, 2024 08:16

modules/zstd: Remove not used absl/status dependency

27aea1c

Internal-tag: [#64294] Signed-off-by: Pawel Czarnecki <[email protected]>

hzeller approved these changes Aug 21, 2024

View reviewed changes

hzeller added the Reviewing Internally label Aug 21, 2024

copybara-service bot merged commit f083652 into google:main Aug 22, 2024
5 checks passed

lpawelcz deleted the 64294-fix-frame-header-tests branch August 22, 2024 06:51

lpawelcz restored the 64294-fix-frame-header-tests branch August 22, 2024 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix frame header CC tests #1550

Fix frame header CC tests #1550

lpawelcz commented Aug 19, 2024

cdleary Aug 19, 2024

lpawelcz Aug 20, 2024

cdleary Aug 19, 2024

lpawelcz Aug 20, 2024

cdleary Aug 19, 2024

lpawelcz Aug 20, 2024

hzeller Aug 19, 2024

lpawelcz Aug 20, 2024

hzeller Aug 20, 2024

proppy Aug 20, 2024

cdleary Aug 21, 2024

cdleary Aug 21, 2024

lpawelcz Aug 21, 2024

hzeller commented Aug 19, 2024

lpawelcz commented Aug 20, 2024

cdleary left a comment

cdleary Aug 20, 2024

lpawelcz Aug 21, 2024

hzeller commented Aug 20, 2024

lpawelcz commented Aug 21, 2024

Fix frame header CC tests #1550

Fix frame header CC tests #1550

Conversation

lpawelcz commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzeller commented Aug 19, 2024

lpawelcz commented Aug 20, 2024

cdleary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzeller commented Aug 20, 2024

lpawelcz commented Aug 21, 2024