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

Fix shuffle test #1454

Merged
merged 2 commits into from
May 17, 2021
Merged

Fix shuffle test #1454

merged 2 commits into from
May 17, 2021

Conversation

Dahlgren
Copy link
Contributor

@Dahlgren Dahlgren commented May 10, 2021

When merged this pull request will:

  • Fix shuffle test

_original = [1, 2, 3];
_result = _original call CBA_fnc_shuffle;
TEST_OP(count _result,==,1,_fn);

_original = [];
_result = [_original] call CBA_fnc_shuffle;
TEST_OP(count _result,==,count _original,_fn);

This comment was marked as resolved.

This comment was marked as resolved.

@commy2
Copy link
Contributor

commy2 commented May 10, 2021

Maybe replace the test with:

_original = [1, 2, 3];
_result = [_original] call CBA_fnc_shuffle;
TEST_OP(count _result,==,1,_fn);

@commy2
Copy link
Contributor

commy2 commented May 10, 2021

If I understand everything correctly, then the original test fails with params expected types script error:

[1, 2, 3] params [["_array", [], [[]]], ["_inPlace", false, [false]]];

converts the 1 to [], and the 2 to false, and drops the 3.
Shuffling the empty array returns an empty array.
The empty array has count 0, which is different from the asserted 1.

This may have broken when adding params to the whole lib. I like the current way it works though, so we should keep the function as is since the change is about 5 years old at least.

@Dahlgren
Copy link
Contributor Author

That's my quick analysis of it as well.

The suggested replacement test seems more or less like the remaining first test, assuming you want 3 as expected count.

_original = [1, 2, 3];
_result = [_original] call CBA_fnc_shuffle;
TEST_OP(count _result,==,count _original,_fn);

@commy2
Copy link
Contributor

commy2 commented May 10, 2021

Ah, I missed that. Could just replace the assert line with

 TEST_OP(_result,isEqualTo,[],_fn); 

@Dahlgren
Copy link
Contributor Author

Sure, will do

@Dahlgren Dahlgren changed the title Remove invalid shuffle test Fix shuffle test May 10, 2021
Co-authored-by: PabstMirror <pabstmirror@gmail.com>
@commy2 commy2 merged commit 6e94a90 into CBATeam:master May 17, 2021
@Dahlgren Dahlgren deleted the bugfix/shuffle-test branch June 17, 2021 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants