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

Handle Jasmine 3.3 #224

Merged
merged 1 commit into from Nov 7, 2018

Conversation

Lalem001
Copy link
Contributor

Manually tested, and confirmed to work with Jasmine@3.0.0 and Jasmine@3.3.0.

Resolves #221

Needs:

  • A way to automate testing in the different versions of Jasmine

@nasvillanueva
Copy link

nasvillanueva commented Oct 30, 2018

Is it an option to use yarn to install multiple versions of jasmine-core? We can have a modified lib/index.js for jasmine-core@3.3.0 tests that would require.resolve version 3.3.0.

Something like

$ yarn add -D jasmine-core330@npm:jasmine-core@3.3.0

// test/main-jasmine-330.js, mainly a copy of lib/index.js
...
var initJasmine = function (files) {
    var jasminePath = path.dirname(require.resolve('jasmine-core330'))
    ...

@johnjbarton Any ideas?

@Havunen
Copy link
Contributor

Havunen commented Nov 4, 2018

One way to test it could be multiple sub projects with own package.json files, that way we could install different versions of Jasmine and do some tests with them

@bricss
Copy link

bricss commented Nov 4, 2018

Any news on this and when it will get landed?

@Roaders
Copy link

Roaders commented Nov 5, 2018

I'm waiting for this fix as well

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Our objective here should be to support all jasmine 3.x. The jasmine team claims there is no API change, but rather that karma-jasmine never set the config correctly in the first place. So our code is not correct for 3.0 or 3.3.

I think we should make a breaking change: call the jasmine 3.x api as intended and not branch to keep compatibility with old API. But the code has to work for both 3.0 and 3.3.

If you agree remove the branching and mark the PR description with BREAKING CHANGE per
http://karma-runner.github.io/3.0/dev/git-commit-msg.html

src/adapter.js Outdated
}
if (jasmineConfig.hasOwnProperty('failFast')) {
jasmineEnv.configure({ failFast: jasmineConfig.failFast })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this setting (failFast -> failFast) different from the old one (stopOnSpecFailure -> failFast)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old version of this source code would call jasmineEnv.stopOnSpecFailure. Per Jasmine.js@3.3.0 L1095-L1098

this.stopOnSpecFailure = function(value) {
  this.deprecated('Setting stopOnSpecFailure directly is deprecated, please use the failFast option in `configure`');
  this.configure({failFast: !!value});
};

Note stopOnSpecFailure replaced with failFast option

src/adapter.js Outdated
}
} else {
setOption(jasmineConfig.stopOnFailure, jasmineEnv.throwOnExpectationFailure)
setOption(jasmineConfig.failFast, jasmineEnv.stopOnSpecFailure)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't these values consistent with the ones above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These values are just indented to fit within the else block. They are otherwise unchanged from the previous version of karma-jasmine

src/adapter.js Outdated
if (typeof jasmineEnv.configure === 'function') {
// it is actually possible to pass jasmineConfig directly to `jasmine.configure`,
// but that would break karma-jasmine's documented use of `stopOnFailure`
// which is not a valid jasmine config option
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this statement true for both versions of Jasmine?

The purpose of this code is to set jasmine config, so we only need to insure that this works for all 3.x. If stopOnFailure is not a jasmine thing in 3.x we don't need to support passing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot find stopOnFailure in Jasmine's source code going back all the way through 1.x.x.

Here is the karma-jasmine commit (0b59d69), and diff, that added stopOnFailure. It appears that before this commit, it was not easy to configure jasmine at all.

spyOn(jasmineEnv, 'randomizeTests')

createStartFn(tc, jasmineEnv)()

expect(jasmineEnv.randomizeTests).toHaveBeenCalledWith(true)
if (jasmineEnv.configuration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Branching in tests is a sign of trouble. Please add a comment explaining why this is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to branch in the tests to work against the live versions of Jasmine. I do see that this kind of testing is more integration than unit testing.

If you would prefer I can mock out the two versions of Jasmine for testing purposes. This would lose out on potential advance warnings of future braking changes from Jasmine.

@Lalem001
Copy link
Contributor Author

Lalem001 commented Nov 6, 2018

@johnjbarton

karma-jasmine never set the config correctly in the first place

Karma-jasmine was passing the jasmine configuration methods around and losing the jasmine binding in the process. The simplest fix, that @nasvillanueva actually had a PR (#223) up for, was to re-bind the method to jasmine. This has drawbacks, mentioned in the PR, and below.

call the jasmine 3.x api as intended and not branch to keep compatibility with old API

Jasmine 3.3.0 adds a new jasmine.configure method that accepts a configuration object. This method simply does not exist on older versions.

Jasmine 3.2.1 had helper methods that would set configuration properties that Karma-Jasmine used. Unfortunately use of these helpers methods result in deprecation warnings in Jasmine@3.3.x.

I elected to avoid the deprecation warnings altogether when switching to 3.3.x, thus branching to jasmine.configure when available.

@johnjbarton
Copy link
Contributor

Thanks for the thorough explanation.

Given all of this I think we should implement the 3.3 API and add a package.json dependency on 3.3, making this a breaking change. WDYT?

@Lalem001
Copy link
Contributor Author

Lalem001 commented Nov 6, 2018

@johnjbarton

Given all of this I think we should implement the 3.3 API and add a package.json dependency on 3.3, making this a breaking change.

I agree. I will work on this after work. Would you prefer a new PR? Or a forced push to this branch/PR?

@johnjbarton
Copy link
Contributor

Just force here is fine, thanks!

Pass `client.jasmine` object directly to `jasmine.configure` method.
See: https://jasmine.github.io/api/3.3/Env.html

Closes: karma-runner#221

BREAKING CHANGE:

`stopOnFailure`, which was previously documented in karma-jasmine's README, is
not configuration option for jasmine. Use `oneFailurePerSpec` instead.

Requires peerDependency Jasmine@^3.3.0
@Lalem001
Copy link
Contributor Author

Lalem001 commented Nov 6, 2018

@johnjbarton Updated the PR. Let me know if you see anything you would like changed.

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and investigation of the issues.

@bricss
Copy link

bricss commented Nov 7, 2018

When we can expect new version? 🤔

@bricss
Copy link

bricss commented Nov 13, 2018

@johnjbarton Humanity depends on this patch, please release in to production 🙏

willmendesneto pushed a commit to willmendesneto/ng-numbers-only that referenced this pull request Nov 16, 2018
## The devDependency [karma-jasmine](https://github.com/karma-runner/karma-jasmine) was updated from `1.1.2` to `2.0.0`.
This version is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

---

<details>
<summary>Release Notes for v2.0.0</summary>

<ul>
<li>feat (adapter): Use jasmine's new configure method (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="375153235" data-permission-text="Issue title is private" data-url="karma-runner/karma-jasmine#224" data-hovercard-type="pull_request" data-hovercard-url="/karma-runner/karma-jasmine/pull/224/hovercard" href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/pull/224">#224</a>) (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/6663e47">6663e47</a>), closes <a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/224" data-hovercard-type="pull_request" data-hovercard-url="/karma-runner/karma-jasmine/pull/224/hovercard">#224</a> <a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/221" data-hovercard-type="issue" data-hovercard-url="/karma-runner/karma-jasmine/issues/221/hovercard">#221</a></li>
</ul>
<h3>Bug Fixes</h3>
<ul>
<li><strong>adapter:</strong> Remove incorrect function and its call. (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/183" data-hovercard-type="pull_request" data-hovercard-url="/karma-runner/karma-jasmine/pull/183/hovercard">#183</a>) (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/cada4a9">cada4a9</a>)</li>
<li>return false for every entry is irrelevant (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/206" data-hovercard-type="pull_request" data-hovercard-url="/karma-runner/karma-jasmine/pull/206/hovercard">#206</a>) (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/d7523d0">d7523d0</a>), closes <a href="https://urls.greenkeeper.io//github.com/karma-runner/karma-jasmine/pull/206/issues/discussion_r186142116">/github.com/karma-runner/karma-jasmine/pull/206#discussion_r186142116</a></li>
<li><strong>console:</strong> Re-add Error: to the stack (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/228" data-hovercard-type="pull_request" data-hovercard-url="/karma-runner/karma-jasmine/pull/228/hovercard">#228</a>) (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/d0b980d">d0b980d</a>)</li>
<li><strong>time:</strong> report correct time since Jasmine v2.9.0 (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/197" data-hovercard-type="pull_request" data-hovercard-url="/karma-runner/karma-jasmine/pull/197/hovercard">#197</a>) (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/022ee04">022ee04</a>), closes <a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/196" data-hovercard-type="issue" data-hovercard-url="/karma-runner/karma-jasmine/issues/196/hovercard">#196</a></li>
</ul>
<h3>Chores</h3>
<ul>
<li><strong>deps:</strong> Drop node v4 support. (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/214" data-hovercard-type="pull_request" data-hovercard-url="/karma-runner/karma-jasmine/pull/214/hovercard">#214</a>) (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/e604132">e604132</a>)</li>
</ul>
<h3>Features</h3>
<ul>
<li>Propagate errors thrown in afterAll blocks (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/f3fa264">f3fa264</a>), closes <a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/issues/161" data-hovercard-type="issue" data-hovercard-url="/karma-runner/karma-jasmine/issues/161/hovercard">#161</a></li>
<li>update the version to 2.0.0 and restrict node version available to 4.0 (<a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/c84316e">c84316e</a>)</li>
</ul>
<h3>BREAKING CHANGES</h3>
<ul>
<li><code>stopOnFailure</code>, which was previously documented in karma-jasmine's README, is<br>
not configuration option for jasmine. Use <code>oneFailurePerSpec</code> instead.</li>
</ul>
<p>Requires peerDependency Jasmine@^3.3.0</p>
<ul>
<li><strong>deps:</strong> Drop support for node 4.x</li>
</ul>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 9 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/2dbec133017572933d02d67a88673d5d67fb227a"><code>2dbec13</code></a> <code>chore: release v2.0.0</code></li>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/4ddfe2ffdf667c66eaddcc405c7042cdde685ddb"><code>4ddfe2f</code></a> <code>chore: update contributors</code></li>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/d0b980db3cb363b7fb0cd48dcd7d529aac83fbca"><code>d0b980d</code></a> <code>fix(console): Re-add Error: to the stack (#228)</code></li>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/6663e4733ad673472bfc4dae7c76d076370e5770"><code>6663e47</code></a> <code>feat (adapter): Use jasmine's new configure method (#224)</code></li>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/e604132ce243f685b5547745b9739c420a294729"><code>e604132</code></a> <code>chore(deps): Drop node v4 support. (#214)</code></li>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/66f598a5fd9b899f655e706576e725b02ccc3de1"><code>66f598a</code></a> <code>docs:(typo) timeoutInterval (#212)</code></li>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/5b45cc12618bf6591bbf8fb3271f2c608927fceb"><code>5b45cc1</code></a> <code>Adding timeout configuration option (#211)</code></li>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/d7523d0d89f89c8c4f454ddd83ef09da1858e207"><code>d7523d0</code></a> <code>fix: return false for every entry is irrelevant (#206)</code></li>
<li><a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/commit/3b20480a5a1cfa769c2a1861455c76d3bfaaab52"><code>3b20480</code></a> <code>--grep option now supports real regular expressions but not masks only. (#204)</code></li>
</ul>
<p>See the <a href="https://urls.greenkeeper.io/karma-runner/karma-jasmine/compare/b670e886dde410b6f28736985036e2ba6c74141e...2dbec133017572933d02d67a88673d5d67fb227a">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
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.

None yet

6 participants