Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Types\to] add unit-tests #1917

Merged
merged 115 commits into from
Mar 4, 2025

Conversation

RickBarretto
Copy link
Collaborator

@RickBarretto RickBarretto commented Mar 1, 2025

Description

Type of change

  • Code cleanup
  • Unit tests (added or updated unit-tests)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (implementation update, or general performance enhancements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (documentation-related additions)

@github-actions github-actions bot added the helpers Issues related to Helper modules label Mar 1, 2025
@RickBarretto RickBarretto changed the title Test type to issue 1354 [Types\to] add unit-tests Mar 1, 2025
@RickBarretto
Copy link
Collaborator Author

@RickBarretto RickBarretto requested a review from drkameleon March 2, 2025 03:05
@RickBarretto
Copy link
Collaborator Author

RickBarretto commented Mar 2, 2025

Everything is set-up. I've just created it into one file, but this should be split into multiple ones in other PR.
Also, some types should be better tested and specific behaviors covered. This also should be done in other PR.

Right now, this covers most of our convertions, and allow us to expand our code without regressions.

Also, this PR depends on #1917. Please merge this after that. 😉

The current situation:

===== Statistics =====

 ⏏️   TOTAL: 84 assertions
✅  PASSED: 84 assertions
⏩ SKIPPED: 217 assertions
❌  FAILED: 0 assertions

===== ========== =====

Those skipped tests reminds me which conversions I should cover as impossible or makes me think about how this should be handled. 😉
This will allow us to fine-tune everything.

Copy link
Collaborator

@drkameleon drkameleon left a comment

Choose a reason for hiding this comment

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

Great job as usual! 🚀 (and a very thorough one at that... 😉 )

P.S.

(I) Just remember that >> is our alias for write :) (I'm not saying that you change it, I'm just saying I was initially confused when I saw it - for internal files, it's perfectly fine; provided we don't... confuse anyone)

(II) If you think it's ready, I'm merging it! 😉 (Not sure, since it's still a draft - asking to make double-sure!)

@RickBarretto
Copy link
Collaborator Author

@drkameleon you're right. I'm thinking about changing it, so I won't confuse anyone. Neither conflict with other tests.

I forgot to mention, but this PR aims #1913 and #1914.

I tried to deprecate them and move their functions to to.
But the complexity was too high to do anything.

So, I decided to initiate the first step of the cycle: test -> refactor -> change.

I can't change it if I'm not sure about the behavior. Also, can't change if I don't know what I'm changing...

I'll use another symbol to tuples tomorrow. And let you know when its finished. 😉

@drkameleon
Copy link
Collaborator

drkameleon commented Mar 4, 2025

I forgot to mention, but this PR aims #1913 and #1914.

Don't worry about it - leave that to me. It's not precisely nothing + I think I could have a good % of it complete until tomorrow, max. (I was planning it actually)

And then you can focus on tests yourself. 😉

I'll use another symbol to tuples tomorrow.

I wouldn't worry too much; I just mentioned it since I went through the code and I'm like "what are we writing where here?" 🤔 lol

And let you know when its finished. 😉

😉

@RickBarretto
Copy link
Collaborator Author

@drkameleon ready to merge?

@RickBarretto RickBarretto requested a review from drkameleon March 4, 2025 18:20
@RickBarretto
Copy link
Collaborator Author

Don't worry about it - leave that to me. It's not precisely nothing + I think I could have a good % of it complete until tomorrow, max. (I was planning it actually)

No worries, at least you can change with without fear. 😉

@drkameleon
Copy link
Collaborator

Let's go for it. Very nice job! 😉

And... ready to merge! 🚀

@drkameleon drkameleon marked this pull request as ready for review March 4, 2025 19:53
@drkameleon drkameleon merged commit 23a5476 into arturo-lang:master Mar 4, 2025
13 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
helpers Issues related to Helper modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Converters] Make sure convertedValueToType works fine + add tests
2 participants