Skip to content

fix: Address flakiness of extractor test #510

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.

Sign up for GitHub

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

Jump to bottom
Merged
victorcolombo merged 3 commits into from
Apr 18, 2023
Merged

fix: Address flakiness of extractor test #510

victorcolombo merged 3 commits into from
Apr 18, 2023

Conversation

@victorcolombo
Copy link
Contributor

@victorcolombo victorcolombo commented Apr 17, 2023

Description:

TestExtractor_Success test shows flakiness due to the divergence on how starlark.Dict is represented internally between test runs. Despite working most of the time, by exhaustively running the test >100 times, you get the following error:

extractor_test.go:42: 
        	Error Trace:	/Volumes/git/kurtosis/core/server/api_container/server/startosis_engine/recipe/extractor_test.go:42
        	Error:      	Not equal: 
        	            	expected: &starlark.Dict{ht:starlark.hashtable{table:[]starlark.bucket{starlark.bucket{entries:[8]starlark.entry{starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x9d275898, key:"another_key", value:starlark.Int{impl:(starlark.intImpl)(0x2ffffffff)}, next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(0x140002b49c8)}, starlark.entry{hash:0x6815c86c, key:"key", value:"value", next:(*starlark.entry)(0x140002b4968), prevLink:(**starlark.entry)(0x140002b49e8)}}, next:(*starlark.bucket)(nil)}}, bucket0:[1]starlark.bucket{starlark.bucket{entries:[8]starlark.entry{starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x9d275898, key:"another_key", value:starlark.Int{impl:(starlark.intImpl)(0x2ffffffff)}, next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(0x140002b49c8)}, starlark.entry{hash:0x6815c86c, key:"key", value:"value", next:(*starlark.entry)(0x140002b4968), prevLink:(**starlark.entry)(0x140002b49e8)}}, next:(*starlark.bucket)(nil)}}, len:0x2, itercount:0x0, head:(*starlark.entry)(0x140002b49a0), tailLink:(**starlark.entry)(0x140002b4990), frozen:false, _:starlark.noCopy{}}}
        	            	actual  : &starlark.Dict{ht:starlark.hashtable{table:[]starlark.bucket{starlark.bucket{entries:[8]starlark.entry{starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x6815c86c, key:"key", value:"value", next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(0x140002b4bc8)}, starlark.entry{hash:0x9d275898, key:"another_key", value:starlark.Int{impl:(starlark.intImpl)(0x2ffffffff)}, next:(*starlark.entry)(0x140002b4b68), prevLink:(**starlark.entry)(0x140002b4be8)}}, next:(*starlark.bucket)(nil)}}, bucket0:[1]starlark.bucket{starlark.bucket{entries:[8]starlark.entry{starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x0, key:starlark.Value(nil), value:starlark.Value(nil), next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(nil)}, starlark.entry{hash:0x6815c86c, key:"key", value:"value", next:(*starlark.entry)(nil), prevLink:(**starlark.entry)(0x140002b4bc8)}, starlark.entry{hash:0x9d275898, key:"another_key", value:starlark.Int{impl:(starlark.intImpl)(0x2ffffffff)}, next:(*starlark.entry)(0x140002b4b68), prevLink:(**starlark.entry)(0x140002b4be8)}}, next:(*starlark.bucket)(nil)}}, len:0x2, itercount:0x0, head:(*starlark.entry)(0x140002b4ba0), tailLink:(**starlark.entry)(0x140002b4b90), frozen:false, _:starlark.noCopy{}}}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -48,2 +48,15 @@
        	            	      (starlark.entry) {
        	            	+      hash: (uint32) 1746258028,
        	            	+      key: (starlark.String) (len=3) "key",
        	            	+      value: (starlark.String) (len=5) "value",
        	            	+      next: (*starlark.entry)(<nil>),
        	            	+      prevLink: (**starlark.entry)({
        	            	+       hash: (uint32) 1746258028,
        	            	+       key: (starlark.String) (len=3) "key",
        	            	+       value: (starlark.String) (len=5) "value",
        	            	+       next: (*starlark.entry)(<nil>),
        	            	+       prevLink: (**starlark.entry)(<already shown>)
        	            	+      })
        	            	+     },
        	            	+     (starlark.entry) {
        	            	       hash: (uint32) 2636601496,
        	            	@@ -53,3 +66,9 @@
        	            	       },
        	            	-      next: (*starlark.entry)(<nil>),
        	            	+      next: (*starlark.entry)({
        	            	+       hash: (uint32) 1746258028,
        	            	+       key: (starlark.String) (len=3) "key",
        	            	+       value: (starlark.String) (len=5) "value",
        	            	+       next: (*starlark.entry)(<nil>),
        	            	+       prevLink: (**starlark.entry)(<already shown>)
        	            	+      }),
        	            	       prevLink: (**starlark.entry)({
        	            	@@ -60,29 +79,6 @@
        	            	        },
        	            	-       next: (*starlark.entry)(<nil>),
        	            	-       prevLink: (**starlark.entry)(<already shown>)
        	            	-      })
        	            	-     },
        	            	-     (starlark.entry) {
        	            	-      hash: (uint32) 1746258028,
        	            	-      key: (starlark.String) (len=3) "key",
        	            	-      value: (starlark.String) (len=5) "value",
        	            	-      next: (*starlark.entry)({
        	            	-       hash: (uint32) 2636601496,
        	            	-       key: (starlark.String) (len=11) "another_key",
        	            	-       value: (starlark.Int) {
        	            	-        impl: (starlark.intImpl) 0x2ffffffff
        	            	-       },
        	            	-       next: (*starlark.entry)(<nil>),
        	            	-       prevLink: (**starlark.entry)(<already shown>)
        	            	-      }),
        	            	-      prevLink: (**starlark.entry)({
        	            	-       hash: (uint32) 1746258028,
        	            	-       key: (starlark.String) (len=3) "key",
        	            	-       value: (starlark.String) (len=5) "value",
        	            	        next: (*starlark.entry)({
        	            	-        hash: (uint32) 2636601496,
        	            	-        key: (starlark.String) (len=11) "another_key",
        	            	-        value: (starlark.Int) {
        	            	-         impl: (starlark.intImpl) 0x2ffffffff
        	            	-        },
        	            	+        hash: (uint32) 1746258028,
        	            	+        key: (starlark.String) (len=3) "key",
        	            	+        value: (starlark.String) (len=5) "value",
        	            	         next: (*starlark.entry)(<nil>),
        	            	@@ -143,2 +139,15 @@
        	            	      (starlark.entry) {
        	            	+      hash: (uint32) 1746258028,
        	            	+      key: (starlark.String) (len=3) "key",
        	            	+      value: (starlark.String) (len=5) "value",
        	            	+      next: (*starlark.entry)(<nil>),
        	            	+      prevLink: (**starlark.entry)({
        	            	+       hash: (uint32) 1746258028,
        	            	+       key: (starlark.String) (len=3) "key",
        	            	+       value: (starlark.String) (len=5) "value",
        	            	+       next: (*starlark.entry)(<nil>),
        	            	+       prevLink: (**starlark.entry)(<already shown>)
        	            	+      })
        	            	+     },
        	            	+     (starlark.entry) {
        	            	       hash: (uint32) 2636601496,
        	            	@@ -148,3 +157,9 @@
        	            	       },
        	            	-      next: (*starlark.entry)(<nil>),
        	            	+      next: (*starlark.entry)({
        	            	+       hash: (uint32) 1746258028,
        	            	+       key: (starlark.String) (len=3) "key",
        	            	+       value: (starlark.String) (len=5) "value",
        	            	+       next: (*starlark.entry)(<nil>),
        	            	+       prevLink: (**starlark.entry)(<already shown>)
        	            	+      }),
        	            	       prevLink: (**starlark.entry)({
        	            	@@ -155,29 +170,6 @@
        	            	        },
        	            	-       next: (*starlark.entry)(<nil>),
        	            	-       prevLink: (**starlark.entry)(<already shown>)
        	            	-      })
        	            	-     },
        	            	-     (starlark.entry) {
        	            	-      hash: (uint32) 1746258028,
        	            	-      key: (starlark.String) (len=3) "key",
        	            	-      value: (starlark.String) (len=5) "value",
        	            	-      next: (*starlark.entry)({
        	            	-       hash: (uint32) 2636601496,
        	            	-       key: (starlark.String) (len=11) "another_key",
        	            	-       value: (starlark.Int) {
        	            	-        impl: (starlark.intImpl) 0x2ffffffff
        	            	-       },
        	            	-       next: (*starlark.entry)(<nil>),
        	            	-       prevLink: (**starlark.entry)(<already shown>)
        	            	-      }),
        	            	-      prevLink: (**starlark.entry)({
        	            	-       hash: (uint32) 1746258028,
        	            	-       key: (starlark.String) (len=3) "key",
        	            	-       value: (starlark.String) (len=5) "value",
        	            	        next: (*starlark.entry)({
        	            	-        hash: (uint32) 2636601496,
        	            	-        key: (starlark.String) (len=11) "another_key",
        	            	-        value: (starlark.Int) {
        	            	-         impl: (starlark.intImpl) 0x2ffffffff
        	            	-        },
        	            	+        hash: (uint32) 1746258028,
        	            	+        key: (starlark.String) (len=3) "key",
        	            	+        value: (starlark.String) (len=5) "value",
        	            	         next: (*starlark.entry)(<nil>),
        	            	@@ -195,11 +187,11 @@
        	            	   head: (*starlark.entry)({
        	            	-   hash: (uint32) 1746258028,
        	            	-   key: (starlark.String) (len=3) "key",
        	            	-   value: (starlark.String) (len=5) "value",
        	            	+   hash: (uint32) 2636601496,
        	            	+   key: (starlark.String) (len=11) "another_key",
        	            	+   value: (starlark.Int) {
        	            	+    impl: (starlark.intImpl) 0x2ffffffff
        	            	+   },
        	            	    next: (*starlark.entry)({
        	            	-    hash: (uint32) 2636601496,
        	            	-    key: (starlark.String) (len=11) "another_key",
        	            	-    value: (starlark.Int) {
        	            	-     impl: (starlark.intImpl) 0x2ffffffff
        	            	-    },
        	            	+    hash: (uint32) 1746258028,
        	            	+    key: (starlark.String) (len=3) "key",
        	            	+    value: (starlark.String) (len=5) "value",
        	            	     next: (*starlark.entry)(<nil>),

After this PR, we rely on both Starlark method to assert equality, removing the dependence of the representation of the dict.

Is this change user facing?

NO

References (if applicable):

gbouv
gbouv approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@gbouv gbouv left a comment

Choose a reason for hiding this comment

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

Nice! Would be curious to learn why the internal dictionary implementation changes from time to time, this does not seem super reassuring, but maybe that's just SL internal stuffs

@victorcolombo
Copy link
Contributor Author

victorcolombo commented Apr 18, 2023

Taking a deeper dive on this @gbouv, I actually found out that the divergence on how starlark.Dict, is implemented, but instead on the output of the Extract function. That happens there is some iteration on maps going in on there, and Go intentionally made map iteration random. See
https://go.dev/doc/go1#iteration

The old language specification did not define the order of iteration for maps, and in practice it differed across hardware platforms. This caused tests that iterated over maps to be fragile and non-portable, with the unpleasant property that a test might always pass on one machine but break on another.

In Go 1, the order in which elements are visited when iterating over a map using a for range statement is defined to be unpredictable, even if the same loop is run multiple times with the same map. Code should not assume that the elements are visited in any particular order.

@victorcolombo victorcolombo enabled auto-merge (squash) April 18, 2023 11:23
@victorcolombo victorcolombo merged commit 4508df3 into main Apr 18, 2023
@victorcolombo victorcolombo deleted the vcolombo/flaky-TestExtractor_Success branch April 18, 2023 11:29
@kurtosisbot kurtosisbot mentioned this pull request Apr 18, 2023
Merged
victorcolombo pushed a commit that referenced this pull request Apr 18, 2023
@kurtosisbot
chore(main): release 0.75.4 (#518)
c9136f3 
🤖 I have created a release *beep* *boop*
---


##
[0.75.4](0.75.3...0.75.4)
(2023-04-18)


### Bug Fixes

* Address flakiness of extractor test
([#510](#510))
([4508df3](4508df3))
* Support ExecRecipe in ReadyCondition
([#507](#507))
([539e8e9](539e8e9))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: kurtosisbot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gbouv gbouv gbouv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gbouv gbouv

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@victorcolombo @gbouv