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

new test for shuffle #636

Merged
merged 2 commits into from Apr 15, 2017
Merged

new test for shuffle #636

merged 2 commits into from Apr 15, 2017

Conversation

@Dorbedo
Copy link
Contributor

Dorbedo commented Apr 10, 2017

commys wish from #635 (depending on this pull request)

@@ -21,18 +21,14 @@ TEST_OP(count _result,==,count _original,_fn);
TEST_OP(_x,in,_original,_fn);
} forEach _result;

// Test depecated version.
// test modify original

This comment has been minimized.

Copy link
@Killswitch00

Killswitch00 Apr 11, 2017

Contributor

Don't remove what the old test checked. Add a new one that modifies the input array.

This comment has been minimized.

Copy link
@Dorbedo

Dorbedo Apr 11, 2017

Author Contributor

The behavior of the functions will change. It won't support the old deprecated Version anymore.
mentioned from commy #635 (comment)

This comment has been minimized.

Copy link
@Killswitch00

Killswitch00 Apr 11, 2017

Contributor

Refactorise the old tests to do what they did before and add a new one that checks if the original array is modified if the function is asked to do that.

This comment has been minimized.

Copy link
@PabstMirror

PabstMirror Apr 11, 2017

Contributor

[1,2,3,4,5,6,7] call CBA_fnc_shuffle;
Old version returns: [4,7,2,1,3,5,6]
New version returns: []

Old version threw a warning to rpt, but still worked.
I am a little worried some people would use it like that.

This comment has been minimized.

Copy link
@Killswitch00

Killswitch00 Apr 11, 2017

Contributor

That aswell - use of the "direct array" parameter form is discouraged, but the function still works. Why break it?

This comment has been minimized.

Copy link
@Dorbedo

Dorbedo Apr 11, 2017

Author Contributor

The warning has been in there since 2009.
My personal opinion is, that the CBA-users should have changed their functions after 8 years of warning.

This comment has been minimized.

Copy link
@Dorbedo

Dorbedo Apr 11, 2017

Author Contributor

@Killswitch00 the new feature is not compatible with the old deprecated behavior

This comment has been minimized.

Copy link
@Killswitch00

Killswitch00 Apr 11, 2017

Contributor

Fair enough (re 8 year warning). The unit test refactorisation need remains, though.

This comment has been minimized.

Copy link
@Dorbedo

Dorbedo Apr 11, 2017

Author Contributor

I'm sorry, but I don't understand, what you want from me.
Could you please be so kind and point it out for me.

This comment has been minimized.

Copy link
@Killswitch00

Killswitch00 Apr 11, 2017

Contributor

Look at the last unit test you altered (line 31). That one is refactorised for the new parameter format. The first two tests in the original version should be kept, but updated in the same way. Then add the new test you added for the "modify input array" option.

@@ -21,18 +21,17 @@ TEST_OP(count _result,==,count _original,_fn);
TEST_OP(_x,in,_original,_fn);
} forEach _result;

// Test depecated version.
_original = [1, 2, 3];
_result = _original call CBA_fnc_shuffle;

This comment has been minimized.

Copy link
@Killswitch00

Killswitch00 Apr 11, 2017

Contributor

No. Update the way the function is called and then do the same two checks that the original unit test performs.

This comment has been minimized.

Copy link
@Dorbedo

Dorbedo Apr 11, 2017

Author Contributor

But it would be the same test like in line 16. So you run the test twice

This comment has been minimized.

Copy link
@Killswitch00

Killswitch00 Apr 11, 2017

Contributor

Didn't see that one. Good. All cases are covered.

@commy2 commy2 added the Enhancement label Apr 12, 2017
@commy2 commy2 merged commit 34b1ecc into CBATeam:master Apr 15, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Dorbedo Dorbedo deleted the Dorbedo:new_shuffle_test branch Apr 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.