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

feat(template-registry-entry): add option to stub template dependencies or reset stubs #27

Closed
wants to merge 4 commits into from
Closed

feat(template-registry-entry): add option to stub template dependencies or reset stubs #27

wants to merge 4 commits into from

Conversation

RomkeVdMeulen
Copy link

See aurelia/testing#20

This adds a stubDependency() function that registers a specific resource path to stub. If a template registry is created for a template that has a <require> for this exact resource, the dependency is ignored.

This is used to isolate composite components during testing. The resetStubbedDependencies() function resets the list of dependencies to ignore.

I'm not exactly sure about the names of these functions, but I couldn't think of names that completely yet succinctly conveys their use and scope. Suggestions are welcome.

@EisenbergEffect
Copy link
Contributor

@RomkeVdMeulen Thanks for working on this! Can you do me a favor? Would you mind removing all the changes under the dist folder so that the PR only includes the src and test changes?

@RomkeVdMeulen
Copy link
Author

Ah right, I forgot.

@EisenbergEffect
Copy link
Contributor

Thanks @RomkeVdMeulen ! Stand by for merge. We just recently changed our CI and branching patterns on this repo, so I'm confirming some of the new procedures with our technical lead before I merge in. Thanks for your patience.

@bigopon
Copy link
Member

bigopon commented Feb 20, 2019

@RomkeVdMeulen It seems odd to me that we are providing this capability in this module, as this, as least to me, feels solely for testing purposes. I was thinking about some patching in testing. Though I haven't done enough of those stubbing to make a reliable conclusion, so maybe you can share some thoughts on why you picked this module to provide this?

@RomkeVdMeulen
Copy link
Author

@bigopon To be honest I hadn't given it much thought: I simply applied the monkey patch we'd discusses directly to the code. Now that you mention it, it isn't very elegant to put code only intended for testing in a place where it'll run in production.

My experience with this stubbing behavior isn't all that impressive, so if you want to try it for yourself it is quite easy to reproduce. Based on what I've tried so far, runtime patching didn't seem to have any adverse effects. If you prefer to add the runtime patch to the aurelia-testing library, I wouldn't be opposed to that either.

The only downside I see is that since the patch and the patched code live in different libraries, it would be very simple for somebody to break the patch by changing TemplateRegistryEntry behavior, not knowing that this patch in aurelia-testing exists. That could be mitigated by adding some comments to the code, but the way I've built it now would actually fail local unit-tests if the behavior breaks: this would probably be easier to support in the long run (although all this will probably change with vNext anyway).

@EisenbergEffect
Copy link
Contributor

I think this module is pretty stable and not likely to change in significant ways going forward. So, I think it would be safer to patch it from the testing library. @RomkeVdMeulen Would you be willing to re-orient this on top of the testing library?

@RomkeVdMeulen
Copy link
Author

Done.

@RomkeVdMeulen RomkeVdMeulen deleted the feat-stub-dependencies branch February 21, 2019 10:35
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.

3 participants