Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Implement wrapper in fusion-test-utils to encapsulate plugin resolution #38

Merged
merged 9 commits into from Jan 16, 2018
Merged

Implement wrapper in fusion-test-utils to encapsulate plugin resolution #38

merged 9 commits into from Jan 16, 2018

Conversation

AlexMSmithCA
Copy link
Member

@AlexMSmithCA AlexMSmithCA commented Jan 13, 2018

Fixes #364

Primary changes:

  • Add getSimulator to encapsulate plugin resolution and expose render and request
  • Add Flow types, where appropriate
  • Update unit tests - includes addition of test with resolution of plugin dependencies
  • Update README.md

src/index.js Outdated
const ctx = renderContext(url, options);
return simulate(app, ctx);
};

export function registerAsTest(
Copy link
Member Author

Choose a reason for hiding this comment

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

Open to thoughts here on the exact API and naming this function. Primary motivation here is to encapsulate resolving the app to allow consumers to have a single app fixture and call .request or .render multiple times (which you cannot do today).

Copy link
Contributor

Choose a reason for hiding this comment

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

createSimulator? createTestDriver?

Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

Mostly looks fine except the dep url.

Not really sure I like the naming, I think something like mockPlugin might be better?

yarn.lock Outdated
@@ -2802,6 +2802,10 @@ fusion-core@0.3.0-2:
node-mocks-http "^1.6.6"
toposort "^1.0.6"

fusion-tokens@^0.0.4:
version "0.0.4"
resolved "https://unpm.uberinternal.com/fusion-tokens/-/fusion-tokens-0.0.4.tgz#b84c58e2de8e06d3e63c2c182da7e023ccfb50ec"
Copy link
Contributor

Choose a reason for hiding this comment

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

💣

@AlexMSmithCA AlexMSmithCA self-assigned this Jan 13, 2018
@codecov
Copy link

codecov bot commented Jan 13, 2018

Codecov Report

Merging #38 into master will increase coverage by 1.67%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   88.88%   90.56%   +1.67%     
==========================================
  Files           3        3              
  Lines          45       53       +8     
  Branches        6        7       +1     
==========================================
+ Hits           40       48       +8     
  Misses          4        4              
  Partials        1        1
Impacted Files Coverage Δ
src/simulate.js 100% <100%> (ø) ⬆️
src/index.js 80.76% <100%> (+10.18%) ⬆️
src/mock-context.js 100% <100%> (ø) ⬆️

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 21a899f...eedcc92. Read the comment docs.

Copy link
Contributor

@dennisgl dennisgl left a comment

Choose a reason for hiding this comment

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

Looks good and I'm going to defer the naming choices to others. However, we do need docs and example on the new helper.

@AlexMSmithCA AlexMSmithCA dismissed stale reviews from dennisgl and KevinGrandon January 16, 2018 22:54

README.md updated


export function getSimulator(app: FusionApp, testPlugin?: FusionPlugin<*, *>) {
if (testPlugin) {
app.register(testPlugin);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we allow for an array of test plugins?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just the public API from the DI system. Whether multiple registration makes sense is something that we can discuss on Giancarlo's new proposal.


[include]
./src/

[libs]
./node_modules/fusion-core/flow-typed
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using this for anything yet? Or just getting our configs to match?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question- it's used here for Context and FusionApp. At least internally, it looks like our plan is to ship libdef files like we are will soon be doing in fusion-core.

@AlexMSmithCA AlexMSmithCA merged commit 395defd into fusionjs:master Jan 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants