Skip to content

.travis:fix up TestShuffle failure on Arm64#12515

Merged
qmonnet merged 1 commit intocilium:masterfrom
Jianlin-lv:pr-fix-shuffle--issue
Jul 14, 2020
Merged

.travis:fix up TestShuffle failure on Arm64#12515
qmonnet merged 1 commit intocilium:masterfrom
Jianlin-lv:pr-fix-shuffle--issue

Conversation

@Jianlin-lv
Copy link
Copy Markdown
Contributor

@Jianlin-lv Jianlin-lv commented Jul 13, 2020

After shuffling, there is a small probability event that
the order of elements has not changed;
The results of multiple tests should prevail.

Signed-off-by: Jianlin Lv Jianlin.Lv@arm.com

Fixes: #12512

@Jianlin-lv Jianlin-lv requested a review from a team as a code owner July 13, 2020 16:18
@maintainer-s-little-helper maintainer-s-little-helper Bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 13, 2020
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 13, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 36.971% when pulling 0386d1a on Jianlin-lv:pr-fix-shuffle--issue into cecf5a7 on cilium:master.

@pchaigno pchaigno added the release-note/misc This PR makes changes that have no direct user impact. label Jul 14, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 14, 2020
@pchaigno pchaigno added area/CI Continuous Integration testing issue or flake dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 14, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 14, 2020
@pchaigno pchaigno added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. ci/flake This is a known failure that occurs in the tree. Please investigate me! labels Jul 14, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 14, 2020
pchaigno
pchaigno approved these changes Jul 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Jianlin-lv!

Am I understanding correctly that this flake isn't actually specific to ARM?

I validated this by running this unit test 10k times in a loop. Without this patch it failed 93/10000 (~1%); with this patch it didn't fail even once 🎉

@Jianlin-lv
Copy link
Copy Markdown
Contributor Author

Jianlin-lv commented Jul 14, 2020

Thanks a lot @Jianlin-lv!

Am I understanding correctly that this flake isn't actually specific to ARM?

I validated this by running this unit test 10k times in a loop. Without this patch it failed 93/10000 (~1%); with this patch it didn't fail even once 🎉

From the experience of fixing multiple Travis issues on the ARM platform,
Most of them are caused by code defects, and are not strongly related to the architecture; it is only easier to expose these issue on the Arm platform.

qmonnet
qmonnet requested changes Jul 14, 2020
View reviewed changes
Comment thread pkg/rand/safe_rand_test.go Outdated
@Jianlin-lv
.travis:fix up TestShuffle failure on Arm64
0386d1a 
After shuffling, there is a small probability event that
the order of elements has not changed;
The results of multiple tests should prevail.

Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
@Jianlin-lv Jianlin-lv force-pushed the pr-fix-shuffle--issue branch from c3b4c6a to 0386d1a Compare July 14, 2020 14:16
@pchaigno pchaigno requested a review from qmonnet July 14, 2020 14:27
qmonnet
qmonnet approved these changes Jul 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@qmonnet qmonnet merged commit b0610a4 into cilium:master Jul 14, 2020
@qmonnet
Copy link
Copy Markdown
Member

qmonnet commented Jul 16, 2020 •
edited
Loading

The for loop with the single retry has been backported to v1.7 and v1.8 (in progress via #12536), I'm marking this commit for backports too so as to limit CI flakes.

@pchaigno pchaigno mentioned this pull request Jul 21, 2020
Merged
@jrfastab jrfastab mentioned this pull request Jul 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pchaigno pchaigno pchaigno approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees

No one assigned

Labels

area/CI Continuous Integration testing issue or flake ci/flake This is a known failure that occurs in the tree. Please investigate me! release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CI: Travis failed on arm64: TestShuffle FAIL

6 participants

@Jianlin-lv @coveralls @qmonnet @pchaigno @jrfastab @christarazi