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

Runner option #68

Merged
merged 2 commits into from
Jun 16, 2016
Merged

Runner option #68

merged 2 commits into from
Jun 16, 2016

Conversation

gwynjudd
Copy link
Contributor

Resoves #65

This option allows the caller to override the default jasmine-runner.js. My use case is to allow me to use the junit-reporter from https://github.com/larrymyers/jasmine-reporters. To use the junit-reporter in phantomjs, it is necessary to make significant changes to the runner script:

It is strongly recommended to use the provided script to run your test suite using PhantomJS. If you want to use your own PhantomJS runner, you will need to inject a __phantom_writeFile method, and also take care to correctly determine when all results have been reported.

@dflynn15
Copy link
Owner

dflynn15 commented Jun 6, 2016

Would you mind checking out PR #62? It seems to be doing the same thing you are going for with a example task and it may be a little cleaner (it doesn't create the runner variable in two places). I want to make sure there isn't anything that you did that would be unique or different.

@gwynjudd
Copy link
Contributor Author

gwynjudd commented Jun 6, 2016

@dflynn15 the change in #62 wouldn't suit my needs - I found that the lib\jasmine-runner.js will exit too early and I had to make fairly significant changes to it to allow the junit-reporter to work correctly

@dflynn15
Copy link
Owner

dflynn15 commented Jun 7, 2016

I am not sure I understand this PR then. Both this PR and #62 allow for an optional reporter to be specified, otherwise the default included in the repository is utilized.

If there is something I am missing please let me know as they both seem to be approaching the same problem.

@gwynjudd
Copy link
Contributor Author

gwynjudd commented Jun 7, 2016

@dflynn15 they do different things. #62 lets you replace the reporter (default /lib/terminal-reporter.js). This lets you replace the runner (default /lib/jasmine-runner.js).

@dflynn15
Copy link
Owner

And this is why you don't look at PRs on your phone... 😭 Quick last, nit-picky request. Could you move the var runner = up instead of having to instantiate it twice?

@gwynjudd
Copy link
Contributor Author

@dflynn15 done

@dflynn15 dflynn15 merged commit a1176ff into dflynn15:master Jun 16, 2016
@dflynn15
Copy link
Owner

Thanks!! getinsertpic.com

@gwynjudd gwynjudd deleted the runner-option branch June 16, 2016 19:30
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.

2 participants