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

core: Add unit test for LogEventSerializer. #431

Merged
merged 15 commits into from
Aug 2, 2024

Conversation

haiqi96
Copy link
Contributor

@haiqi96 haiqi96 commented Jun 9, 2024

References

Description

Added unit test for LogEventSerializer and Deserializer

The unit test does the following:

  1. Encode two log events into an IR file
  2. Decodes the two log events from the IR file and convert them to plain text log message
  3. Verify that the encoded log_events and decoded log events are identical

Validation performed

New test is added to the unit test and passed

@haiqi96 haiqi96 changed the title Draft: Adding unit test for LogEventSerializer Adding unit test for LogEventSerializer Jun 10, 2024
@haiqi96 haiqi96 marked this pull request as draft June 10, 2024 15:32
@haiqi96 haiqi96 marked this pull request as ready for review June 11, 2024 20:17
@haiqi96 haiqi96 changed the title Adding unit test for LogEventSerializer core: Adding unit test for LogEventSerializer Jun 11, 2024
@haiqi96 haiqi96 changed the title core: Adding unit test for LogEventSerializer core: Add unit test for LogEventSerializer Jun 12, 2024
@LinZhihao-723
Copy link
Member

Before adding review comments, I think it's worth discussing the high-level concern here. The changes in decoding_methods.hpp/cpp should be moved to another PR for the following reasons:

  1. The code added in this PR is duplicating generic_deserialize_log_event. The method itself is quite useful though (both javascript ffi and IR v2 require such a deserialize function). But we shouldn't simply duplicate code to create another function.
  2. The current namings are quite confusing. We should distinguish which of these methods are deserialize_xxx, decode_xxx, and deserialize_and_decode_xxx respectively. In this PR, we have two overloaded deserialize_log_event, but they are not doing the same thing.

Copy link
Member

@LinZhihao-723 LinZhihao-723 left a comment

Choose a reason for hiding this comment

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

I will prefer to rewrite the test case to have log events to serialize stored in a vector (push whatever u have right now), and just loop through the vector for serialization/deserialization. The enumeration in this test case seems too verbose

components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
@haiqi96 haiqi96 requested a review from LinZhihao-723 August 1, 2024 19:00
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_serializer.cpp Outdated Show resolved Hide resolved
@kirkrodrigues kirkrodrigues changed the title core: Add unit test for LogEventSerializer core: Add unit test for LogEventSerializer. Aug 1, 2024
@haiqi96 haiqi96 enabled auto-merge (squash) August 1, 2024 23:34
@haiqi96 haiqi96 merged commit f38eee9 into y-scope:main Aug 2, 2024
13 checks passed
jackluo923 pushed a commit to jackluo923/clp that referenced this pull request Dec 4, 2024
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.

3 participants