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.

Sign up for GitHub

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

Jump to bottom

Add unit-tests to node::Taffy #178

Merged
merged 23 commits into from
Jun 19, 2022
Merged

Add unit-tests to node::Taffy #178

merged 23 commits into from
Jun 19, 2022

Conversation

Weibye
Copy link
Collaborator

@Weibye Weibye commented Jun 18, 2022 •
edited
Loading

Objective

Fixes #177

Context

As I understand it:

  • For a test to access private and internal behaviour, it needs to be in the same module as the definition it is trying to test.
  • Unit-tests are to ensure correct and sound internal behavior.

So as a result I have moved all tests from ./tests/node.rs to ./src/node.rs and expanded upon them to cover more of the internals. I've created a discussion on this topic #179 where we can discuss this in detail.

Changelog

  • All tests relating to node::Taffy has been moved to mod::node
  • ./test/node.rs has been removed
  • Added more tests to cover previously untested behavior

Feedback wanted

  • Overall discussion on where to keep what tests: Guidelines for testing in `taffy` #179
  • There are some tests that I currently have commented out since they don't really work but I'm not quite sure how to test for those, or if we need to test for those at all.

@Weibye Weibye added the code quality Make the code cleaner or prettier. label Jun 18, 2022
@Weibye Weibye marked this pull request as ready for review June 19, 2022 08:36
Weibye
Weibye commented Jun 19, 2022
View reviewed changes
src/node.rs Show resolved Hide resolved
src/node.rs Outdated Show resolved Hide resolved
TimJentzsch
TimJentzsch suggested changes Jun 19, 2022
View reviewed changes
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.

Please move the tests to a tests module (in the same file). You can take a look at the MaybeMath tests as reference.

The tests itself look good to me though.

src/node.rs Outdated Show resolved Hide resolved
src/node.rs Outdated Show resolved Hide resolved
TimJentzsch
TimJentzsch reviewed Jun 19, 2022
View reviewed changes
src/node.rs Outdated Show resolved Hide resolved
src/node.rs Show resolved Hide resolved
src/node.rs Outdated Show resolved Hide resolved
src/node.rs Outdated Show resolved Hide resolved
@Weibye Weibye mentioned this pull request Jun 19, 2022
Closed
TimJentzsch
TimJentzsch reviewed Jun 19, 2022
View reviewed changes
src/node.rs Outdated Show resolved Hide resolved
alice-i-cecile
alice-i-cecile requested changes Jun 19, 2022
View reviewed changes
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.

Happy to merge once Tim's comment is resolved.

@TimJentzsch
Copy link
Collaborator

As a tip: You can use cargo test --no-default-features locally to run the tests without default features enabled, then you don't have to wait for CI every time (and it might clean up the commits a bit) :)

@Weibye
Copy link
Collaborator Author

Weibye commented Jun 19, 2022

As a tip: You can use cargo test --no-default-features locally to run the tests without default features enabled, then you don't have to wait for CI every time (and it might clean up the commits a bit) :)

That I didn't know, thanks!

Weibye
Weibye commented Jun 19, 2022
View reviewed changes
Copy link
Collaborator Author

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

Hang on, going to migrate tests to new_leaf

@Weibye
Copy link
Collaborator Author

Weibye commented Jun 19, 2022

All comments should now be resolved :)

@alice-i-cecile alice-i-cecile merged commit e8a32c5 into DioxusLabs:main Jun 19, 2022
@Weibye Weibye deleted the taffy-node-unit-tests branch June 19, 2022 16:02
jkelleyrtp pushed a commit that referenced this pull request Oct 10, 2022
@Weibye
Add unit-tests to node::Taffy (#178)
61e0842 
* Creating scaffolding for new tests

* Moving and adding tests

* Fleshing out more tests

* Add test for find-node

* fix import using #[cfg]

* adding child at index text

* Adding child-count test

* 0-index consistency pass on naming

* adding children test

* Finishing up last tests

* revert uneccesary changes

* Move tests inside `tests` module + fix comment placement

* Removing unnecessary test

* fix type of vec?

* std::vec -> sys::vec

* Further attempt at fixing vec

* Exposing some methods so we can use them for testing

* Fix vec type

* Add helper comment

* attempting pushing to vec

* Rebase onto new changes

* Cleanup

* Migrate tests to new_leaf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimJentzsch TimJentzsch TimJentzsch approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
code quality Make the code cleaner or prettier.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unit test Node
3 participants
@Weibye @TimJentzsch @alice-i-cecile