Skip to content

restore and fix gentest#145

Merged
alice-i-cecile merged 9 commits intoDioxusLabs:mainfrom
mockersf:fix-gentest
Jun 11, 2022
Merged

restore and fix gentest#145
alice-i-cecile merged 9 commits intoDioxusLabs:mainfrom
mockersf:fix-gentest

Conversation

@mockersf
Copy link
Copy Markdown
Contributor

Objective

Comparing with a browser implementation is the simplest way to ensure results will match what is expected. This can't be done by fuzzing or prop testing as both would mean recoding a flexbox implementation in tests to generate the assertions to validate the "random" values used.

#141 removed the gentlest script which ensure the tests generated from Chrome are up to date. I strongly disagree with this.

Fixes #65

  • I re-added and fixed the gentest script
  • I regenerated the tests. There was a few errors that are probably due to changes in Chrome impl since the last time the tests were run (most likely 3 years ago?)
    • rounding of fractional values has been simplified
    • do not use the minimum size to compute the basis value for the size. this caused an issue when specifying a growth factor as the free space was not correctly computed. I also fixed the doc comment on the default value for flex-grow

About the comments on maintaining the generated tests. I believe this was a pain because they were updated manually as the script was failing. Now that it's working again, the tests should not be edited by hand: update the script and regenerate them. They can be ignored during reviews. It would probably be best to commit them in a separate commit to help reviewers
If needed, this could be improved using GitHub actions:

  • generate the tests from a GitHub action, and commit them from the action in their own PR
  • or just check that the tests in the PR matches the one from a fresh generation to ensure no manual edit happened

@alice-i-cecile alice-i-cecile added bug Something isn't working code quality Make the code cleaner or prettier. build system Make continuous integration do the tedious things labels Jun 11, 2022
@alice-i-cecile
Copy link
Copy Markdown
Collaborator

Can you add those bug fixes to RELEASES.md so users know about the functional changes?

@alice-i-cecile
Copy link
Copy Markdown
Collaborator

Another request: can you update CONTRIBUTING.md to properly describe how to generate these tests, and add your review advice?

@mockersf
Copy link
Copy Markdown
Contributor Author

done!

@alice-i-cecile alice-i-cecile merged commit b2d8bda into DioxusLabs:main Jun 11, 2022
jkelleyrtp pushed a commit that referenced this pull request Oct 10, 2022
* fix gentest

* regenerate tests

* fix doc comment (default value)

* simplify rounding

* do not take minimum size for flex-basis

* revert deleted files

* rename

* document fixes

* update doc on gentest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working build system Make continuous integration do the tedious things code quality Make the code cleaner or prettier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix cargo run --package gentest

2 participants