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

Adds test for methods in vec, map and bytes (#840) #900

Merged
merged 5 commits into from
Jun 14, 2023

Conversation

Ray-Escobar
Copy link
Contributor

What

Adds tests for the methods within the modules Vec, Map and Bytes.

Tests have been added for the following methods...

Bytes: is_empty, first, first_unchecked, last, last_unchecked, get, get_unchecked, remove, remove_unchecked, pop, pop_unchecked, insert, slice

Map: from_array, contains_key, is_empty, get, get_unchecked, remove, remove_unchecked

Vec: get, get_unchecked, remove, remove_unchecked

Why

These modules are the building blocks of soroban-sdk.

Extra

I refactored some tests names in Vec so that they are pre-pended with the word test. It seems the majority of the tests follow this convention, but I also stopped short in case the renaming is not necessary. I do find its useful as an easy way to search files i.e. if you want to look for tests simply use the query "find_" on all your files.

@Ray-Escobar Ray-Escobar force-pushed the tests-vec-map-bytes branch from 98298bc to 303ce33 Compare March 30, 2023 10:14
@Ray-Escobar
Copy link
Contributor Author

On a side note, there are some println! statements in lines 1144 and line 1165 in the tests for bytes.rs. Should I go ahead and remove them?

@jayz22
Copy link
Contributor

jayz22 commented Mar 31, 2023

@Ray-Escobar thanks for the PR! I will take a look at the tests soon.
As for print statements, we normally don't have them (either active or commented out) in our code base, so I would suggest removing them.

@Ray-Escobar
Copy link
Contributor Author

@jayz22 thanks! I am very open with feedback so be as critical as you need.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests!

If possible I'd like us to change how we assert the panics so that we're asserting on the specific function that we expect to panic. See inline comment.

soroban-sdk/src/bytes.rs Outdated Show resolved Hide resolved
@Ray-Escobar Ray-Escobar force-pushed the tests-vec-map-bytes branch from 303ce33 to 1b7f130 Compare May 17, 2023 09:52
Adds tests for the methods within the modules Vec, Map and Bytes.

These modules are the bulding blocks of soroban-sdk.
@Ray-Escobar Ray-Escobar force-pushed the tests-vec-map-bytes branch from 1b7f130 to 5c0b6a9 Compare June 3, 2023 11:34
@leighmcculloch leighmcculloch enabled auto-merge (squash) June 14, 2023 02:04
@leighmcculloch leighmcculloch changed the title tests: Adds test for methods in vec, map and bytes (#840) Adds test for methods in vec, map and bytes (#840) Jun 14, 2023
@leighmcculloch leighmcculloch disabled auto-merge June 14, 2023 02:13
@leighmcculloch leighmcculloch enabled auto-merge (squash) June 14, 2023 02:13
@leighmcculloch leighmcculloch merged commit 9fe3217 into stellar:main Jun 14, 2023
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.

3 participants