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 test suite #270

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

illia-li
Copy link

No description provided.

marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/utils/selected.go Outdated Show resolved Hide resolved
marshal/tests/utils/string.go Outdated Show resolved Hide resolved
marshal/tests/utils/utils.go Outdated Show resolved Hide resolved
marshal/tests/utils/utils.go Outdated Show resolved Hide resolved
Comment on lines 38 to 39
for i := range s.Values {
val := s.Values[i]
Copy link
Collaborator

Choose a reason for hiding this comment

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

	serialization.Set{
		Data:   []byte("\x00\x00"),
		Values: mod.IntoRef.Append("0"),
	}.Run("[0000]str", t, marshal, unmarshal)

In my opinion, it will be easier and clearer this way. And Set.Run function become easier and clearer.

Sure, let's do this then:

 	serialization.Set{
 		Data:   []byte("\x00\x00"),
 		Values: mod.AddVariants([]interface{}{"0"}, mod.IntoRef),
 	}.Run("[0000]str", t, marshal, unmarshal)

marshal/tests/utils/selected.go Outdated Show resolved Hide resolved
marshal_3_smallint_test.go Show resolved Hide resolved
@illia-li
Copy link
Author

Changes for serialization.Set was pushed, please check it for further work on the rest of the.

marshal/tests/funcs/default.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Show resolved Hide resolved
marshal/tests/serialization/set.go Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Show resolved Hide resolved
@illia-li illia-li force-pushed the il/add/marshal_test_suite branch 2 times, most recently from f7d0c24 to 8a452e2 Compare September 24, 2024 17:44
dkropachev
dkropachev previously approved these changes Sep 24, 2024
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/funcs/new.go Outdated Show resolved Hide resolved
marshal/tests/mod/all.go Outdated Show resolved Hide resolved
marshal/tests/mod/all.go Outdated Show resolved Hide resolved
marshal/tests/serialization/pointers.go Outdated Show resolved Hide resolved
marshal/tests/serialization/pointers.go Outdated Show resolved Hide resolved
marshal/tests/serialization/pointers.go Outdated Show resolved Hide resolved
@illia-li
Copy link
Author

Please check it out, if everything is fine, I will squash the commits and continue working on test suite for the corrupt cases

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.

Bunch of small nit picks, last pack.
@sylwiaszunejko , can you please also take a look.

marshal/tests/mod/custom_refs.go Outdated Show resolved Hide resolved
marshal/tests/funcs/equal.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
marshal/tests/utils/string.go Outdated Show resolved Hide resolved
marshal/tests/utils/utils.go Outdated Show resolved Hide resolved
marshal/tests/utils/utils.go Outdated Show resolved Hide resolved
marshal/tests/serialization/set.go Outdated Show resolved Hide resolved
@illia-li illia-li mentioned this pull request Sep 25, 2024
@sylwiaszunejko
Copy link
Collaborator

not related to the code, but please squash commits, so they are sets of distinctive changes not "resolve conversation" ones

@sylwiaszunejko
Copy link
Collaborator

I unresolved two comments that I don't think were addressed, but maybe I am missing something there are a lot of changes

@illia-li illia-li force-pushed the il/add/marshal_test_suite branch 2 times, most recently from 7b4f030 to bd403b5 Compare September 26, 2024 09:56
@illia-li
Copy link
Author

not related to the code, but please squash commits, so they are sets of distinctive changes not "resolve conversation" ones

I did it, divided by packages.
There were a lot of changes and the code changed first one way, then the other, in such a situation it is very difficult to do something differently

I unresolved two comments that I don't think were addressed, but maybe I am missing something there are a lot of changes

Fixed

@sylwiaszunejko
Copy link
Collaborator

Thanks for addressing the comments! I would like you to fix the commit history a little bit, commits like "cleaning", "optimizing x", "simplify x" should not exist, squash them and make the first commits clean/optimal/simple etc. in the first place. Having such commits with fixes that could be incorporated into the original commits is not something we want to merge into our git history

@illia-li
Copy link
Author

illia-li commented Sep 26, 2024

Thanks for addressing the comments! I would like you to fix the commit history a little bit, commits like "cleaning", "optimizing x", "simplify x" should not exist, squash them and make the first commits clean/optimal/simple etc. in the first place. Having such commits with fixes that could be incorporated into the original commits is not something we want to merge into our git history

Something like this?

@sylwiaszunejko
Copy link
Collaborator

Right now, it's a single large commit. Ideally, it would be better to break it into smaller commits, but not in the way it was before, like "adding A" and then "optimizing part x of A." Instead, it should be more like "adding a" and then "adding b (not related to a)". For example, in this PR (#256), each commit focuses on adding or fixing one specific thing. If a commit is fixing something, it's from the master branch or was added before that PR, not something introduced in the same PR. Does that make sense?

dkropachev
dkropachev previously approved these changes Sep 26, 2024
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.

Very last nits.
Let's also wait till @sylwiaszunejko approve it.

@@ -0,0 +1,18 @@
package funcs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this code into utils package.

Comment on lines 25 to 26
BrokenMarshalTypes []interface{}
BrokenUnmarshalTypes []interface{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Last optimization,

Suggested change
BrokenMarshalTypes []interface{}
BrokenUnmarshalTypes []interface{}
BrokenMarshalTypes []reflect.Type
BrokenUnmarshalTypes []reflect.Type

return out.Interface()
}

func EqualTypes(in1, in2 interface{}) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since reflect.Type is comparable, please replace it with reflect.TypeOf(val1) == reflect.TypeOf(val2)

@illia-li
Copy link
Author

Right now, it's a single large commit. Ideally, it would be better to break it into smaller commits, but not in the way it was before, like "adding A" and then "optimizing part x of A." Instead, it should be more like "adding a" and then "adding b (not related to a)". For example, in this PR (#256), each commit focuses on adding or fixing one specific thing. If a commit is fixing something, it's from the master branch or was added before that PR, not something introduced in the same PR. Does that make sense?

In general, it makes sense.
But this PR does not change the current code at all, it is adding test suite.
This can be done in different ways, just tell me how.

resolve conversation v1...resolve conversation vn was needed so that the github would not lose conversations.

@dkropachev
Copy link
Collaborator

Right now, it's a single large commit. Ideally, it would be better to break it into smaller commits, but not in the way it was before, like "adding A" and then "optimizing part x of A." Instead, it should be more like "adding a" and then "adding b (not related to a)". For example, in this PR (#256), each commit focuses on adding or fixing one specific thing. If a commit is fixing something, it's from the master branch or was added before that PR, not something introduced in the same PR. Does that make sense?

In general, it makes sense. But this PR does not change the current code at all, it is adding test suite. This can be done in different ways, just tell me how.

resolve conversation v1...resolve conversation vn was needed so that the github would not lose conversations.

I also agree that there is no reason to have it splitted into commits, even though it has 15 files, all of them a there to do exact one thing, which is in line with the commit message.

@dkropachev
Copy link
Collaborator

dkropachev commented Sep 26, 2024

@illia-li , please don't resolve conversations, answer in the thread instead.

internal/tests/utils/new.go Outdated Show resolved Hide resolved
internal/tests/utils/equal.go Outdated Show resolved Hide resolved
@illia-li
Copy link
Author

We will continue working on the corrupt tests here?

marshal/tests/serialization/pointers.go Outdated Show resolved Hide resolved
marshal/tests/serialization/pointers.go Outdated Show resolved Hide resolved
marshal/tests/serialization/utils.go Outdated Show resolved Hide resolved
@dkropachev
Copy link
Collaborator

We will continue working on the corrupt tests here?

No, let's merge it in and continue on another PR.

@illia-li
Copy link
Author

Let`s merge it:)

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.

@sylwiaszunejko , can you pleae take another look, i am ok with this PR.

@sylwiaszunejko
Copy link
Collaborator

@illia-li Thanks for your work!

@sylwiaszunejko sylwiaszunejko merged commit 25c97f8 into scylladb:master Sep 27, 2024
1 check passed
@dkropachev
Copy link
Collaborator

@illia-li Thanks you, sorry for being so picky

@illia-li illia-li deleted the il/add/marshal_test_suite branch September 27, 2024 13:06
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