Fix expected result set of transform test #640
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
keiko713
commented
Nov 26, 2024
| @@ -134,7 +134,7 @@ func TestStatements(t *testing.T) { | |||
| } | |||
| expectedJSON, _ := json.Marshal(expected) | |||
|
|
|||
| // Query: 1, 0, Plan: 0, 1 | |||
| // Query: 1, 0, Plan: 0, 1 (w/ QueryIdx 0) | |||
There was a problem hiding this comment.
note: the "bug" part is; I forgot that I was using the second query as a plan's query, so the QueryIdx of QueryPlanReference does change (previously, it was using the fixed number 1 here).
Now QueryPlanReference has the right set of combinations.
|
Closing in favor of #639 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The test was having the wrong expected result set, so I updated it. Without this fix, test was consistently failing (e.g. if I run test 10 times, it guaranteed to fail a few times), but with this fix, I ran test 50 times and it was working fine (I also triple checked the logic).
#630 (comment)
I agree with others that the current testing logic is not ideal and also very hard to maintain. I thought about removing this completely, but it's a nice test to have and it was still fixable for now (as it had a clear bug) so I went forward and fixed it. We shouldn't expand this test any further and if we want to add something, we should reconsider how we do the test.
I'm not so sure if the sorting works, this test is passing input to the blackbox (
transform.StateToSnapshot) and we don't know in which combination the blackbox is going to return the result with.