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 async resultbuilder-based support #1194

Closed
wants to merge 1 commit into from
Closed

Conversation

younata
Copy link
Member

@younata younata commented Jan 12, 2023

This basically serves to reimplement Quick using a ResultBuilder approach. It doesn't integrate with anything else in Quick, and I only provide basic BeforeEach, It (the actual test), AfterEach, as well as example groups.

This should serve as a proof of concept for this approach. I want to test out how this scales before fully committing to it.

The PR should summarize what was changed and why. Here are some questions to
help you if you're not sure:

  • What behavior was changed?
  • What code was refactored / updated to support this change?
  • What issues are related to this PR? Or why was this change introduced?

Checklist - While not every PR needs it, new features should consider this list:

  • Does this have tests?
  • Does this have documentation?
  • Does this break the public API (Requires major version bump)?
  • Is this a new feature (Requires minor version bump)?

@github-actions
Copy link

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@younata younata force-pushed the async_resultbuilder branch 7 times, most recently from 7bbe994 to 16814b7 Compare January 12, 2023 07:10
This basically serves to reimplement Quick using a ResultBuilder approach.
It doesn't integrate with anything else in Quick, and I only provide basic beforeEach, it (the actual test), afterEach, as well as example groups.

This should serve as a proof of concept for this approach.
I want to test out how this scales before fully committing to it.

Assuming we like this, then I'll commit to adding other features of the DSL to this.
@younata
Copy link
Member Author

younata commented Jan 14, 2023

Ok, I've spent enough time with this to know that, sadly, this is not a viable path forward.

Positives:

  • Having spec be a class method works out quite well. No need to new up a blank instance of QuickSpec to add the test methods. It might be worthwhile to migrate QuickSpec to have spec be a class method. It's a very simple migration we could provide a sed command for.
  • Arbitrary strings (so long as they start with "test") as test selectors works very well. 100% should apply this to QuickSpec and whatever AsyncSpec looks like next.
  • Up to a point, putting the DSL as class methods on AsyncSpec works quite well. Once the test gets decently large, though, then Swift ran into issues resolving types. This could maybe be resolved by having the DSL for synchronous specs (in Swift) be accessible/implemented as class methods on QuickSpec.
  • ResultBuilder is quite easy to get started with.
  • Compile-time checking that the DSL is used mostly correctly is awesome.

Negatives:

  • ResultBuilder-based specs do not scale. Eventually, you start running in to "Could not resolve types" issues once an AsyncSpec becomes large enough.
  • You cannot embed functions or type declarations inline in the spec.
  • You need to assign all declared variables (i.e. can't just have var foo: MyType!, you must use var foo: MyType! = nil). Thankfully, this limitation will be removed in an upcoming version of swift.

The lack of scaling is a showstopper. But frankly the last two are also large-enough issues that this approach is untenable.

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.

None yet

1 participant