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 broken test in Captains Log #2588

Merged
merged 4 commits into from
Nov 28, 2023
Merged

Conversation

santialb
Copy link
Contributor

@santialb santialb commented Nov 28, 2023

Changed the end index of the randomShipRegistryNumber() so that the range goes from 1000 to 9999

Fixes #2589


Reviewer Resources:

Track Policies

Changed the end index of the randomShipRegistryNumber() so that the range goes from 1000 to 9999
Copy link
Contributor

@sanderploegsma sanderploegsma left a comment

Choose a reason for hiding this comment

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

Great start, and I appreciate your willingness to try and contribute!

Unfortunately it looks like you made a small typo when updating CaptainsLog.java, see the inline comment.

Next to that the test class also needs updating. If you make sure to commit all changes to the same branch (called patch-1 in your copy of this repository) they should all end up in this same Pull Request. 👍

Made changes to the Test file updating the expected values to (which are NCC-6258, NCC-1683 and NCC-4922 respectively).
Updated the typo of the end range to be 10000 instead of 11000
Copy link
Contributor Author

@santialb santialb left a comment

Choose a reason for hiding this comment

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

I made the changes to the previous PR, changing the end range to 10000 and changing the test file to the new expected values (which are NCC-6258, NCC-1683 and NCC-4922 respectively).

Copy link
Contributor

@sanderploegsma sanderploegsma left a comment

Choose a reason for hiding this comment

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

Great work, thanks for fixing this!

@sanderploegsma sanderploegsma changed the title Update CaptainsLog.java Fix broken test in Captains Log Nov 28, 2023
@sanderploegsma sanderploegsma merged commit 424c8c4 into exercism:main Nov 28, 2023
5 checks passed
@sanderploegsma
Copy link
Contributor

Congratulations with your first contribution @santialb! 🎉

@santialb
Copy link
Contributor Author

Thanks for letting me help @sanderploegsma !

@santialb santialb deleted the patch-1 branch November 29, 2023 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Captain's Log, typo in the instructions or error in the test for task 2
2 participants