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

Shallow-render should now support both Jest & Jasmine #100

Merged
merged 4 commits into from
Apr 5, 2019

Conversation

duck-nukem
Copy link
Collaborator

@duck-nukem duck-nukem commented Apr 5, 2019

Should fix #99

Couple of points:

  1. There's a note about being type safe with public contracts - I wanted to add, that the implementations for wrapping jest & jasmine are now using any in more places than I'd like. Since it's used internally, I could settle with it, but in the current implementation this is exposed as a public static property on the Shallow class, so it's available for consumers of this library. I wanted to avoid installing packages that'd help with the typings, but I can see that as an option.

  2. Test coverage - I have added exactly 0 tests for these changes, since I've found it difficult to test for global objects/namespaces. Also, in the library itself, there's nothing at the moment ensuring that tests run with jest (I did test it on the example that I've attached in Adding jest support #99 though). The only way I can think of is to set up sample projects that use different testing frameworks.

  3. I think I explicitly went against "No funny business" with the way I'm trying to determine if jest is available or not, although I'd let you be the judge of that. I've checked ts-mockery (as suggested in Adding jest support #99 ) and that package seems to accept a configuration for which test runner to use. I'm being selfish, and I know I wouldn't want to set that up with shallow-render if it can be avoided, but not sure if the current implementation is better than allowing external configuration.

  4. Tried to come up with an implementation that's somewhat in line with https://github.com/getsaf/shallow-render/projects/1#card-18903827 - not sure how well I did on that

@getsaf
Copy link
Owner

getsaf commented Apr 5, 2019

Great work! I think you covered things nicely and I like that the framework is auto-detected. I think it's fine to declare jest/jasmine as any since it's internal and we don't know the user's version.

I think that since the user should be interacting with jest/jasmine directly in their tests so we should consider keeping the testingFrameworks object private to shallow-render. I couldn't imagine an end-user writing a test with Shallow.testingFramework.spyOn and wouldn't want to encourage it.

What do you think about converting the public static property to a singleton module that can be used internally like:

import { testFramework } from './lib/test-framework';
testFramework.spyOn();

This could be accomplished by converting the getAvailableTestFramework function in lib/test-framework to export the singleton like this:

// I've taken some liberties in reducing the detection logic you used in `getAvailableTestFramework`
export const testFramework = typeof jest === 'undefined'
  ? new JasmineFramework()
  : new JestFramework();

I think we could probably put out a pre-release NPM package that we can use to test out in a sample project with Jest.

Thanks again for the PR, glad to have your support!

@duck-nukem
Copy link
Collaborator Author

Thanks for reviewing my PR, I've extracted the public class property to a singleton module as you suggested.

I'd be happy to keep contributing to this project 🎉

@getsaf
Copy link
Owner

getsaf commented Apr 5, 2019

Cool, gonna test our your changes. I noticed some weirdness in the TSC output with an import, I think I have a fix and I'll submit a PR to your branch when I'm done.

Once this is all merged, I'll add you to the contributors list!

@getsaf getsaf merged commit eab5583 into getsaf:master Apr 5, 2019
@getsaf
Copy link
Owner

getsaf commented Apr 5, 2019

Looks like you already resolved the import issue in your latest change. This tests well, I created an angular 6 project and tests ran with Karma and Jest!

@kylecannon
Copy link
Contributor

Awesome guys. I'll test this when I get some time as well.

@duck-nukem duck-nukem deleted the add-jest-support branch November 17, 2022 20:48
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.

Adding jest support
3 participants