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

Expand Shell validation tests #1714

Merged
merged 14 commits into from
Mar 22, 2023
Merged

Expand Shell validation tests #1714

merged 14 commits into from
Mar 22, 2023

Conversation

hannobraun
Copy link
Owner

Expand the Shell validation tests by also testing the "valid" scenario, not just the "invalid" one. Expand the builder API to support that.

This is a step towards #1713, but it doesn't quite get us where we need to. The new builder APIs are too specialized to be of any real use outside of what they're currently used for. They definitely need some more work.

On the plus side, all previous attempts to write builder APIs like this either turned out overly complicated, or failed outright. So the recent cleanup work really did help. This also gave me some new ideas on how to evolve the builder API further, and I'll try those out next.

Also test the validation check with valid geometry. This is very
verbose, as we're lacking the right APIs to build `Shell`s. I'm working
on changing that now.
This is a bit overly complicated, as not all the code changed here is
actually improved by the `SurfaceBuilder`. However, keeping those
different cases consistent makes the next refactoring steps easier.
The old names were inherited from the test were this code was originally
written, and they no longer fit this more generic context.
@hannobraun hannobraun enabled auto-merge March 22, 2023 12:02
@hannobraun hannobraun merged commit 2b80cc3 into main Mar 22, 2023
@hannobraun hannobraun deleted the builder branch March 22, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant