Skip to content

Conversation

@morganzellers
Copy link
Contributor

@morganzellers morganzellers commented Nov 3, 2021

Linked Issue:

Checklist:


If Applicable:

  • Did you test when the first item is skipped?
  • Did you test when the last item is skipped?
  • Did you test when middle items are skipped?
  • Did you test when incorrect data is passed forward?
  • Did you test proceeding backwards?

If Public API Has Changed:

  • Did you deprecate (rather than remove) any old methods/variables/etc? Our philosophy for deprecation.
  • Have you done the best that you can to make sure that the compiler guides people to changing to the new API? (Example: the renamed attribute)
  • If necessary, have you tested the upgrade path for at least N-1 versions? For example, if data persists between v1 and v2 then that upgrade should be tested and as easy as we can make it.

morganzellers and others added 5 commits November 2, 2021 11:09
@morganzellers morganzellers added the enhancement New feature or request label Nov 3, 2021
@morganzellers morganzellers added this to the Data Driven Workflows milestone Nov 3, 2021
@morganzellers morganzellers self-assigned this Nov 3, 2021
@morganzellers morganzellers changed the base branch from main to data-driven November 3, 2021 18:47
Copy link
Collaborator

@Richard-Gist Richard-Gist left a comment

Choose a reason for hiding this comment

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

I'm going to try to stay hands-off, and let you run with this. Reach out if you want some help.

wait(for: [exp], timeout: TestConstant.timeout)
}

func testLaunchingAWorkflowFromAnAnyWorkflow() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll want to either update this or split it out. This is effectively only testing that we can compile with multiple FRs in the workflow, and that we can get an FR on the screen.

The updated and/or new test would be something with FR1-4 and each FR is a different type (one is Passthrough, one is String/String, One is String/Never, Never/Never) or something like that. Then we'll want to actually assert that we can step through the entire flow and find FR1-4 on the screen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's exactly what the generic constraint tests are for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tyler-Keith-Thompson I'll take a look at those in the AM 👍🏻

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2021

Codecov Report

Merging #150 (1f62b9b) into data-driven (7ac2343) will decrease coverage by 1.56%.
The diff coverage is 63.23%.

❗ Current head 1f62b9b differs from pull request most recent head 9a283fd. Consider uploading reports for the commit 9a283fd to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           data-driven     #150      +/-   ##
===============================================
- Coverage        91.27%   89.70%   -1.57%     
===============================================
  Files               92       94       +2     
  Lines             2373     2497     +124     
===============================================
+ Hits              2166     2240      +74     
- Misses             207      257      +50     
Impacted Files Coverage Δ
...wiftCurrent/Models/FlowRepresentableMetadata.swift 100.00% <ø> (ø)
...urrent_SwiftUI/Extensions/WorkflowExtensions.swift 45.67% <44.30%> (-54.33%) ⬇️
Sources/SwiftCurrent/TypeErased/AnyWorkflow.swift 81.66% <66.66%> (-0.79%) ⬇️
...tUI/Models/ExtendedFlowRepresentableMetadata.swift 72.72% <72.72%> (ø)
...ftCurrent_SwiftUI/TypeErased/AnyWorkflowItem.swift 80.00% <80.00%> (ø)
.../SwiftCurrent_SwiftUI/Views/WorkflowLauncher.swift 98.96% <96.15%> (-1.04%) ⬇️
...rces/SwiftCurrent_SwiftUI/Views/WorkflowItem.swift 94.07% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ac2343...9a283fd. Read the comment docs.

@morganzellers
Copy link
Contributor Author

Added a test for fatalError when no items in a workflow earlier then spent most of the day deciphering some of the unit tests and working through adding needed thenProceed's.

@morganzellers
Copy link
Contributor Author

EOD: Was able to drive out a few more thenProceed's today. Starting to understand them more, but am still fuzzy at times on which situation in a workflow a thenProceed signature translates to.

Also fuzzy on the difference between setting typealias WorkflowOutput = String vs typealias WorkflowOutput = AnyWorkflow.PassedArgs. I thought that setting the latter basically said "Whatever type I'm passed is what I'm passing on" but there were a few times today where I doubted my memory.

@morganzellers
Copy link
Contributor Author

EOD: Added support for startingArgs - swapped to using the GenericConstraintsTests (copied and renamed for ExtendedFlowRepresentableMetadata) to drive out more functionality for persistence and launch style arguements.

@Richard-Gist
Copy link
Collaborator

I tried to work on this a bit today, but there's a LOT of work we'll need to do to convert all of the tests over to actually test things. I tried some different routes like maybe taking the generic constraint tests from SwiftCurrent core and seeing if those were easier to change over, but they weren't because of a difference in what you can use as starting arguments.

Maybe someone with a bit more regex prowess can make mass edits to convert the code, but for now, the only way forward I see is essentially translating all of the tests. I found I had to update how the workflow was created, how it was launched, and how it was asserted.

I did contemplate for a while if we even needed to do all of this, which is still a "maybe" to a somewhat of a "yeah". We might be able to prune it down some but a lot of these tests will remain valid if we ever get to a point of letting our users declare Workflows and feed them into SwiftUI this way (which is something that holds a lot of value for integrating into certain design patterns).

But overall I'm not sure we're actually going to use these thenProceeds to build out our workflows from data.

morganzellers and others added 3 commits November 11, 2021 12:41
… needed to test launching with a workflow. Added TODO for the thenProceeds to be removed later. - RAG MZ

Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
@morganzellers morganzellers marked this pull request as ready for review November 11, 2021 19:42
…vate again. - RAG MZ

Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
Copy link
Collaborator

@Richard-Gist Richard-Gist left a comment

Choose a reason for hiding this comment

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

Even though the Linter is failing, I am approving this PR because we are merging into a feature branch. Those TODO failures will prevent us from accidentally merging into main with that code, but also allow us to move forward with getting data-driven working.

@Richard-Gist Richard-Gist merged commit de9fe1c into data-driven Nov 11, 2021
@Richard-Gist Richard-Gist deleted the launch-with-workflow branch November 11, 2021 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants