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

add marshal corrupt test suite #278

Merged

Conversation

illia-li
Copy link

@illia-li illia-li commented Sep 28, 2024

Changes:

  1. Added corrupt test suite for testing marshal, unmarshal functions.
  2. Added corrupt test for smallint
  3. Joined serialization, utils packages to internal/tests/serialization

@illia-li illia-li force-pushed the il/add/marshal_corrupt_test_suite branch from 418ae94 to b1a4ee3 Compare September 28, 2024 13:05
@illia-li illia-li mentioned this pull request Sep 28, 2024
@dkropachev dkropachev self-requested a review September 29, 2024 14:44
marshal_3_smallint_corrupt_test.go Outdated Show resolved Hide resolved
marshal_3_smallint_test.go Show resolved Hide resolved
internal/tests/serialization/set.go Outdated Show resolved Hide resolved
internal/tests/serialization/set.go Outdated Show resolved Hide resolved
internal/tests/serialization/set.go Outdated Show resolved Hide resolved
@illia-li illia-li force-pushed the il/add/marshal_corrupt_test_suite branch 2 times, most recently from 4ecfdd1 to 68b4b03 Compare September 29, 2024 20:16
@illia-li illia-li force-pushed the il/add/marshal_corrupt_test_suite branch 2 times, most recently from 4943365 to 45d7cf0 Compare September 29, 2024 23:07
@sylwiaszunejko
Copy link
Collaborator

sorry for being so picky about commit messages but move all code to internal/tests/serialization is not the best name, all code should be replaced with something more specific

@illia-li illia-li force-pushed the il/add/marshal_corrupt_test_suite branch from 45d7cf0 to 14bc33e Compare September 30, 2024 16:24
@illia-li
Copy link
Author

sorry for being so picky about commit messages but move all code to internal/tests/serialization is not the best name, all code should be replaced with something more specific

Is it better now?

}

// RunCorruptTest runs tests for cases when the function should an error.
func (s Set) RunCorruptTest(name string, t *testing.T, marshal func(interface{}) ([]byte, error), unmarshal func([]byte, interface{}) error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In order to avoid confusiton let's have another structure for these tests or, even split it into two (for negative serialization and deserialization tests), since you already logically split them:
Set -> PositiveSerializationSet
NegativeMarshalSet
NetativeUnmarhsalSet

Feel free to suggest better names for them, or provide contr argument why we hsould not do it.

Copy link
Author

Choose a reason for hiding this comment

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

:) It's more difficult than just writing code.

Copy link
Author

Choose a reason for hiding this comment

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

Done

`serialization` and `utils` packages are joined and moved to `internal/tests/serialization`
@illia-li illia-li force-pushed the il/add/marshal_corrupt_test_suite branch from 14bc33e to 6a1a6be Compare October 1, 2024 01:33
Copy link
Collaborator

@dkropachev dkropachev left a comment

Choose a reason for hiding this comment

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

Let's wait for @sylwiaszunejko approve

@sylwiaszunejko sylwiaszunejko merged commit e5191bd into scylladb:master Oct 1, 2024
1 check passed
@illia-li illia-li deleted the il/add/marshal_corrupt_test_suite branch October 1, 2024 13:28
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