Skip to content

Remove redundant src field from Sample#316

Merged
Wondertan merged 1 commit intomasterfrom
hlib/fix-test
May 10, 2021
Merged

Remove redundant src field from Sample#316
Wondertan merged 1 commit intomasterfrom
hlib/fix-test

Conversation

@Wondertan
Copy link
Member

@Wondertan Wondertan commented May 10, 2021

Originally Sample stored src field not to share math/rand object. After migration to crypto/rand that became redundant, though kept within the code, as previously unnoticed. The code also had the undesired side effects that caused TestSampleSquare to randomly fail. This PR fixes both issues.

@Wondertan Wondertan self-assigned this May 10, 2021
@Wondertan Wondertan mentioned this pull request May 10, 2021
Merged
@Wondertan
Copy link
Member Author

Wondertan commented May 10, 2021

e2e tests are successful, so I think we can ignore other failed as flaky

@liamsi
Copy link
Member

liamsi commented May 10, 2021 •
edited
Loading

The code also had the undesired side effects that caused TestSampleSquare to randomly fail.

@Wondertan Can you briefly explain how the src field caused TestSampleSquare to fail. It's not obvious to me.

@Wondertan
Copy link
Member Author

Wondertan commented May 10, 2021 •
edited
Loading

@liamsi, yeah

The test checks that samples are always not equal. However, the test was still failing, as sometimes equal samples were generated, due to src field being taken into account for Sample equality by GO's map, but Sample.Equals only looked at Col and Row fields. Hope that makes sense 😅

@liamsi
Copy link
Member

liamsi commented May 10, 2021

as sometimes equal samples were generated, due to src field being taken into account for Sample equality by GO's map

Oh I think see: https://github.com/lazyledger/lazyledger-core/blob/50f722a510dd2ba8e3d31931c9d83132d6318d4b/p2p/ipld/sample.go#L61

while: https://github.com/lazyledger/lazyledger-core/blob/50f722a510dd2ba8e3d31931c9d83132d6318d4b/p2p/ipld/sample_test.go#L30

the map uses the src field and equality does not. Makes sense.

liamsi
liamsi approved these changes May 10, 2021
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Great find. LGTM

@Wondertan Wondertan merged commit 8be8c36 into master May 10, 2021
@Wondertan Wondertan deleted the hlib/fix-test branch May 10, 2021 09:35
cmwaters pushed a commit that referenced this pull request Mar 13, 2023
@thanethomson
Release v0.34.27-alpha.1 (#316)
38abebd 
* Add changelog entry for #314

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Build changelog with unreleased changes for pre-release

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* version: Set to v0.34.27-alpha.1

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Update to incorporate/backport changes from #317

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Simplify release workflow further

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Add step descriptions for pre-release and release workflows to explain what they do

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liamsi liamsi liamsi approved these changes

@evan-forbes evan-forbes Awaiting requested review from evan-forbes

@tac0turtle tac0turtle Awaiting requested review from tac0turtle

Assignees

@Wondertan Wondertan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Wondertan @liamsi