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

Remove listener #149

Merged
merged 2 commits into from
Jan 31, 2019
Merged

Remove listener #149

merged 2 commits into from
Jan 31, 2019

Conversation

battk
Copy link
Contributor

@battk battk commented Jan 23, 2019

Currently jasmine-npm does not remove the exit listener it adds when it executes. This pull moves the exit listener code completely within the completion reporter so that the listener can be added and removed whenever jasmine is started and done. This is basically pull #125 , except that the jasmine interface remains as intact as possible. The only interface change is that the completion reporter now uses jasmineStarted to add the exit listener. The jasmineStarted method can no longer be overwritten without losing the exit check logic. You can close issue #134 after merging. The fix should allow jasmine to be used with things like gulp watch without errors about max listeners.

Unfortunatly I also had to make failing tests pass on Windows since file logic relied on path separators only being / when windows also allows \. The fix will probably make it so that on windows, files aren't required twice. I don't actually think requiring a file twice does any harm, especially with how it is used here.

Unrelated, but I found it ironic that the tests were smart enough to remove the exit listener while the actual code it was testing was not.

@slackersoft slackersoft merged commit 8e9ab27 into jasmine:master Jan 31, 2019
slackersoft pushed a commit that referenced this pull request Jan 31, 2019
- Merges #149 from @battk
- Fixes #134
- Fixes #125
@slackersoft
Copy link
Member

Looks good. The tests knew they didn't want to actually mess with the outer on exit based on the inner Jasmine being run, and that there might be multiple specs running an entire Jasmine inside Jasmine's suite, so they would need to be removing the handler. Jasmine still really expects to be booted once, have the specs loaded, execute specs, then exit, so wasn't expecting the case where things are run multiple times.

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