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

Fix jest test runner #371

Closed
wants to merge 4 commits into from
Closed

Fix jest test runner #371

wants to merge 4 commits into from

Conversation

SMJ93
Copy link
Contributor

@SMJ93 SMJ93 commented Oct 25, 2017

Fix jest test runner on Android and iOS.

Handle no configuration flags in the Jest runner.

Should fix #362 and #363

New up to date demo project for react native which uses jest as a test runner.

To run:

cd examples/demo-react-native-jest
yarn
npm run test-android
npm run test-ios

@SMJ93
Copy link
Contributor Author

SMJ93 commented Oct 26, 2017

@rotemmiz One of the builds failed because of an issue with npm, could you manually trigger another build?

network request to https://registry.npmjs.org/babel-plugin-syntax-exponentiation-operator/-/babel-plugin-syntax-exponentiation-operator-6.13.0.tgz failed, reason: getaddrinfo ENOTFOUND registry.npmjs.org registry.npmjs.org:443

Copy link
Contributor

@Kureev Kureev left a comment

Choose a reason for hiding this comment

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

Can you please separate the fix itself from example so it is easier to review?

reuse: program.reuse,
debugSynchronization: program.debugSynchronization,
artifactsLocation: program.artifactsLocation
configuration: program.configuration || '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you stick to defaults that commander use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the flags are '' the defaults will be used by the commander - similar to mocha.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just saying that instead of using || '' everywhere in the execSync command you can specify defaults to
program.options in the beginning of the file. It supports three parameters in .option chain:

program.option('-flag', 'description', 'defaults')

so your || '' can be re-written in a more elegant way. For instance,

program.option('-l, --log-level', 'Log level ...', '')

where third '' is a default value.

Does it make sense?

@SMJ93
Copy link
Contributor Author

SMJ93 commented Oct 30, 2017

@Kureev, this PR just includes the jest test runner fix.

#370 includes the new demo project.

@SMJ93
Copy link
Contributor Author

SMJ93 commented Oct 31, 2017

@Kureev , I have updated the commander defaults.

Copy link
Contributor

@Kureev Kureev left a comment

Choose a reason for hiding this comment

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

🎉

@Kureev
Copy link
Contributor

Kureev commented Nov 1, 2017

Hi @rotemmiz! I think this one is ready to be merged

@ferrannp
Copy link

ferrannp commented Nov 2, 2017

I can confirm this is needed for Jest runner to work.

@sibelius
Copy link

sibelius commented Nov 6, 2017

let's do it

@rotemmiz
Copy link
Member

rotemmiz commented Nov 12, 2017

Hey @SMJ93, thank you very much for this PR!
Sorry for taking that long to respond, we've been really busy with releasing new features in our core product.

There are two issues I wanted to discuss before we merge it:

  1. I am not sure I understand what is the issue this PR fixes, what happens if Jest receives empty configuration ? argparse should be able to handle empty keys or no keys, and querying it should be done carefully (if (argparse.getArgValue('key'))). It can return either null, undefined, or empty string, but they should all resolve to false.
  2. Jest integration seems to be catching pretty fast, and I want to make sure it will not get broken easily. If we indeed want to fully support it, let's add another build to our build Matrix that will run the demo projects (including both Mocha and Jest test projects)

@SMJ93
Copy link
Contributor Author

SMJ93 commented Nov 13, 2017

@rotemmiz no worries!

Regarding your issues:

  1. Previously, passing through empty configuration to Jest resulted in strange behaviour; the build failed and verbose was always enabled.
    To fix this we added default values so it would fallback if null, undefined etc. There are probably better ways to do fix this - I will try to find some time to look into argparse, but for the time being this fixes Jest.

  2. Yeah, for us (and probably many others!) it makes sense to use the same test runner for both unit and e2e tests. Yeah that sounds like a better solution! If you have time at some point maybe we could go through the build matrix?

@rotemmiz
Copy link
Member

Sorry @SMJ93 , I won't accept a workaround solution. We either need to understand what causes Jest to act weirdly (probably they way argparse is being consumed around argparse.getArgValue('loglevel')).

@rotemmiz
Copy link
Member

@SMJ93 , can I use the email in your github account to invite you to our slack group ?

@SMJ93
Copy link
Contributor Author

SMJ93 commented Nov 13, 2017

@rotemmiz, I understand what you're saying, but at the moment the Jest test runner is broken on both iOS and Android. This would allow people to use it for the time being while we find a better fix / understand the problem better.

Yeah sure. I'll be on after work.

@sibelius
Copy link

@rotemmiz what is missing in this PR?

@rotemmiz
Copy link
Member

Hey @sibelius , my comment above #371 (comment). This PR will probably won't make it into master.

We are actually doing a few changes internally, that will include fixes for this issue, and a few others with Jest, as preliminaries to a bigger task we plan to publish soon, parallelization support for detox tests...

@SMJ93
Copy link
Contributor Author

SMJ93 commented Nov 20, 2017

@rotemmiz, is there an ETA for this fix?

@rotemmiz
Copy link
Member

A few breaking changes in the configuration, but we believe its a sane and more usable API than what we have today.
#423

I'm closing this PR as what it tried to solve was solved in the PR 423.

@rotemmiz rotemmiz closed this Nov 21, 2017
@wix wix locked and limited conversation to collaborators Jul 23, 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.

Problems on Android using Jest test runner
5 participants