Skip to content

Add unit-tests to node::Taffy#178

Merged
alice-i-cecile merged 23 commits intoDioxusLabs:mainfrom
Weibye:taffy-node-unit-tests
Jun 19, 2022
Merged

Add unit-tests to node::Taffy#178
alice-i-cecile merged 23 commits intoDioxusLabs:mainfrom
Weibye:taffy-node-unit-tests

Conversation

@Weibye
Copy link
Copy Markdown
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
Comment thread src/node.rs
Comment thread src/node.rs Outdated
TimJentzsch
TimJentzsch suggested changes Jun 19, 2022
View reviewed changes
Copy link
Copy Markdown
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.

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

@Weibye Weibye force-pushed the taffy-node-unit-tests branch from 50fb660 to 00cfab5 Compare June 19, 2022 15:26
@TimJentzsch
Copy link
Copy Markdown
Collaborator

TimJentzsch 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) :)

@Weibye
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

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

+1 more reviewer

@TimJentzsch TimJentzsch TimJentzsch approved these changes

Reviewers whose approvals may not affect merge requirements

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