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

Add unit-tests to resolve #173

Merged
merged 2 commits into from
Jun 17, 2022
Merged

Conversation

Weibye
Copy link
Collaborator

@Weibye Weibye commented Jun 16, 2022

Objective

A tiny step towards #92, but mostly I need to see where things are breaking when working on #160 and building unit-test helps.

Feedback wanted

Not quite sure if these are considered good tests as they feel a tiny bit arbitrary in terms of which magical numbers to test for, so any feedback is welcome :)

@alice-i-cecile alice-i-cecile added the code quality Make the code cleaner or prettier. label Jun 16, 2022
Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

The cases are not that fun to review because the lines are so long that they wrap and GitHub and they don't have syntax highlighting, but we probably can't fix that.

Anyway, seems solid for me. The resolve_size one has a lot of cases, I wonder if the property based testing frameworks that @alice-i-cecile recommended would help with that? I haven't looked into them yet.

In any case, it's good to see more unit tests, that will definitely help with refactoring and optimizations!

Copy link
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Not the prettiest, but I think this is good for now.

Merging to make refactors easier.

@alice-i-cecile alice-i-cecile enabled auto-merge (squash) June 17, 2022 00:26
@alice-i-cecile alice-i-cecile merged commit 12d44d5 into DioxusLabs:main Jun 17, 2022
@mcrvaz
Copy link
Contributor

mcrvaz commented Jun 17, 2022

I was thinking of doing some tests just like this ones for the remaining public functions in this file.
Is this the expected test format?
Should we keep tests in the same file as the feature or separate them?

@alice-i-cecile
Copy link
Collaborator

I was thinking of doing some tests just like this ones for the remaining public functions in this file.

Yes please!

Is this the expected test format?

This seems like a nice format; we can use it for consistency but feel free to raise concerns that you have.

Should we keep tests in the same file as the feature or separate them?

For unit tests of internal functionality like this, keep them in the same file as they're defined. Integration tests belong in the root level test folder.

@Weibye Weibye deleted the test-resolve branch June 18, 2022 12:46
jkelleyrtp pushed a commit that referenced this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Make the code cleaner or prettier.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants