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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to jasmine 4 #105

Open
mjeanroy opened this issue Feb 13, 2022 · 3 comments
Open

Upgrade to jasmine 4 #105

mjeanroy opened this issue Feb 13, 2022 · 3 comments

Comments

@mjeanroy
Copy link
Contributor

Hi @sindresorhus,

Would you be interested in a PR to migrate to Jasmine 4?
If you're ok, I can work on this and submit a PR, otherwise no worries 馃槈

Thanks!

@sindresorhus
Copy link
Owner

馃憤

@jsuddsjr
Copy link

The issues, explained

I spent several hours yesterday taking a look at the updates in this branch and how the gulp-jasmine tests interact with the Jasmine 4 engine. Here's what I found:

  1. For performance reasons, Jasmine engine is using a Singleton pattern for its environment. It doesn't matter how many times you call new Jasmine(options), you will get the same instance unless you force a shutdown in between.

  2. By setting jasmine.exitOnCompletion = false, you are disabling the shutdown. (A premature shutdown can prevent promises from completing, so it's an engineering workaround on their end.)

  3. Each spec file is represented as a "suite" of tests internally. Duplicate files are skipped to avoid duplication of effort, resulting in overall status of "incomplete" from the engine. (IMO: "skipped" might have been a better choice.) You know this has happened when you get the following verbose output.

    Running 0 specs. 
    0 specs, 0 failures. 
    Finished in 0 seconds.
    
  4. And here's the gut punch: The Jasmine engine returns one "overall status" for all suites combined.

    • This happens asynchronously, no matter which spec file you think you are running. So, even if the current spec passes all tests, the engine might still return an overall result of "failed."
    • The information returned from execute does not tell you which suite caused the failure, so you'd have to crack open the engine to map failures correctly.

I know the Jasmine folks have deprecated engine shutdown, but without a granular way to determine which part of the suite is reporting errors, this seems premature IMO. (This might have changed in Jasmine 5 alpha, more investigation needed.)

Why this matters

None of these changes would matter if all the gulp-jasmine tests were designed to pass, but this is not the case. The fail-fixture.js is specifically designed to test what happens when tests fail. (Excellent choice, BTW.)

NOTE These issues are not likely to impact regular users of gulp-jasmine as test failures generally should be reported.

  1. The ava tests are assuming that each fixture is run in isolation. (False.)
  2. Only the failure tests are configured to ignore errors. (Failure are reported across the entire suite.)
  3. Files are submitted multiple times to test different features. (Duplicates are skipped.)

Some architectural suggestions

In order to run a comprehensive suite of tests for gulp-jasmine, you might consider the following suggestions.

  • Allow the caller to customize the default "exitOnCompletion = false" setting, making it possible to require a shutdown between invocations of the gulp-jasmine pipeline. Then, enforce a shutdown between ava tests.

  • Write a second ava suite to run the fail-fixture.js tests separately from those that succeed. Manually restart Jasmine engine between these two ava suites.

A note about promise rejection

Finally, just a reminder that await will throw when a promise is rejected for any reason. The code in index.js is wrapped in a try/catch, but the catch handler turns around and sends a PluginError back to the flush callback, causing a secondary "unhandled exception" in the transform pipeline. This might be the desired effect for production pipelines that need to crash when a test fails, but it begs the question how to detect these edge cases more gracefully in the ava tests.

  • Add setting to configure pipeline to return 'jasmineError' events rather than PluginError exceptions. This would allow the pipeline owner to more gracefully detect and respond to pipeline failures caused by rejected promises.

@sindresorhus
Copy link
Owner

If anyone wants to work on this, see the initial attemp in #106

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 a pull request may close this issue.

3 participants