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

馃挭 KILLING IT with the Jasmine unit tests 馃挭 #1072

Merged
merged 26 commits into from Oct 17, 2017

Conversation

Projects
None yet
2 participants
@pcraig3
Copy link
Contributor

pcraig3 commented Oct 13, 2017

馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭

馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭

馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭

馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭
馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭馃挭

馃挭馃挭馃挭馃挭馃挭

In what can only be described as a spectacular triumph against overwhelming odds, I've boldly converted a practically uncountable number of unit tests (ie, 405 [see above for a visual aid]) that were originally written for Jasmine 1.x to work with Jasmine 2.8. I've also made them a bit more robust in places by fixing the test code so that we can run them in a browser should we so choose.

We are agents of history: the choice is ours.

Finally finally spotlight is running in node 6 with the full suite of tests intact from when it was running on node 0.10.0. As Winston Churchill once said: "Never have so many tests been updated so throughly across so many breaking versions."

May they pass for another 10,000 releases.

pcraig3 added some commits Sep 20, 2017

Update jshint and fix syntax errors
Updating the node version broke jshint for some reason, but
fortunately not very badly.

Found the relevant fix here: jshint/jshint#2922 (comment)

With jshint working again, I went back and fixed some of the
syntax errors I'd introduced.
Return jshint to grunt unit tests
Back in 300c8ea, I took out some
broken tests so that we would have a deployable project.

Fixed the jshint problem, so now it's coming back into our grunt
unit tests.
Remove all but one 'spec.*' file
Should be easier to get the jasmine tests running by starting out
with a smaller number of files.
Upgrade grunt-contrib-jasmine
Means that the jasmine version we're using for our client tests
is v2.x, whereas the jasmine version we're using for the
server/shared tests is v1.x

We also have to upgrade phantomjs as part of this.

- the old version of jasmine won't run the old version of phantomjs
- the new version of jasmine doesn't work with the old version of
  phantomjs
- the new version of jasmine works with the new version of
  phantomjs but now all the tests are broken
Downgrade d3
I was getting some weird errors in the tests, so I've just
downgraded the library to remove any "what if" scenarios.
Remove duplicated parameter
Wasn't being used. Has to be nested inside "options".
Update vendor/helper libraries used for testing
Updated 1 library and 2 helpers

- app/vendor/jquery
    - from: http://code.jquery.com/jquery-1.11.1.js
- spec/helpers/jasmine-jquery_helper
    - from: https://www.npmjs.com/package/jasmine-jquery
- spec/helpers/moment_helper

The first three I just dropped in based on googling for the most
current version of the file. The fact that they're not package
managed is going to cause a similar issue in 3 years when they're
once again super out of date.

The `moment_helper.js` one wasn't a vendored file, but one that's
been written just for use in this project. Accordingly,  I've just
rewritten it based on the instructions at:
https://jasmine.github.io/2.0/upgrading.html
'spec/client/common/views/visualisations/*' tests working
Fixed a bunch of updated syntax in various test files.
All pretty trivial stuff.

Reference: https://jasmine.github.io/2.0/upgrading.html
Comment out jquery-deparam until we resolve issues with it
This also means commenting out tests that need jquery-deparam
Handle edge case that throws d3 error
This was working fine when jasmine was run from the command line
but failing when run in a browser.
As near as I can tell, it represents an edge case we wouldn't
normally expect, so I fixed it.
'spec/client/controllers/*' tests working
Fixed a bunch of updated syntax in various test files.
All pretty trivial stuff.

Reference: https://jasmine.github.io/2.0/upgrading.html
Rewrite index file so that jasmine is booting properly
Since Jasmine 2.x, the way to boot jasmine initally has changed
quite a bit and our `spec.testrunner.js` file wasn't cutting it.

Ordered the require statements a bit differently, mostly based on
this stakoverflow response: https://stackoverflow.com/a/20851265

After this point, it's all about converting the tests to the new
jasmine so that they run as before.

Got 7 tests passing so far from the /spec/spec.*.js test files.

馃挭馃挭馃挭馃挭馃挭馃挭馃挭
'spec/client/extensions/*' tests working鈥
Didn't need to update anything, luckily.
'spec/client/views/*' tests working
Mostly fixing syntax: https://jasmine.github.io/2.0/upgrading.html

Excluded the tests that make use of jquery-deparam because I don't
have that working as of yet.

A few remarks:

spec.services-kpi.js
- in this file, I needed to instantiate a window.GOVUK.analytics
  object (same as in spec.table.js)

spec.services-kpi.js
- `this.removeAllSpies();` is deprecated (it no longer needs to
  be manually called), even though it's not obvious in the
  documentation on upgrading jasmine

`spec.visitors-realtime.js`
- I put the template HTML into the test setup as a string so
  that the tests work in a browser as well as from the command
  line. Otherwise, the browser complains about a cross-origin
  request problem
'spec/client/views/views/graph/*' tests working
Mostly this consisted in fixing syntax, as per:
Reference: https://jasmine.github.io/2.0/upgrading.html

However, a few other issues emerged:

spec.graph.js
- for some reason, running the tests in a browser fail when d3
  is expected to caclulate the inital size of an svg inside a
  wrapper element. While this works fine in phantomjs, it was
  failing consistently in different browsers.
- since I don't know a lot about the code these tests are testing,
  I ended up just putting in an if-condition to only run the tests
  if they are being run in phantom; browsers will just skip them
- worth mentioning that the resizing behaviour of the graphs
  themselves in the live app and they were working as expected

- error templates are not able to be retrieved during browser
  testing; trying to load the template from another directory is
  a prohibited cross-origin request
- added more conditional logic to tests looking for the missing-data
  template so that they will not be run in browsers

spec.line-set.js
- because of variable hoisting, we have to include the `el: undefined`
  argument in the expectation.
- the base Backbone.View object adds an "el" key to the passed-in 'options'
  argument at the very end of the View::initialize method, which jasmine
  then sees as part of the initial argument passed to the View object.
- so we have to "expect" that an `el: undefined` argument was passed,
  whether it was or not

spec.linelabel.js
- same as in `spec.graph.js`, some initial d3 calculations fail in
  browsers, so skip them if test suite is running in a browser
- `createSpy` syntax seems to have changed, so I've updated it to
  work with Jasmine 2.x (this wasn't made clear in the docs)

spec.stack-set.js
- same as in `spec.line-set.js`, we have to expect an `el: undefined`
  because of javascript's variable hoisting

spec.stacked-linelabel.js
- same as in `spec.graph.js` and `spec.linelabel.js`, conditionally
  skip tests in browsers that rely on initial d3 element sizes
Visualisation fallback tests not working
Here's the deal though, it doesn't look like they are working
in production either.
Currently, loading a service page in IE8 (in browserstack)
doesn't seem to actually give you an image.
'spec/client/views/visualisations/**/*' tests working
Reference: https://jasmine.github.io/2.0/upgrading.html

A few remarks:

spec.number.js
- `collection.dataSource.setQueryParam` was throwing an exception
  so I created a spy on the method that was getting called to just
  return the value we wanted
- this follows a common pattern in the same file of creating spies
  to return expected values so it seemed to be a good solution

spec.target.js
- error templates are not able to be retrieved during browser
  testing; trying to load the template from another directory is
  a prohibited cross-origin request
- added conditional logic to test looking for the missing-data
  template so that it will not be run in browsers
Prefix helper files for client/server tests
So this is the hilarious result of isomorphic javascript.

Here's the deal:

`grunt jasmine` tests were using jasmine 1.x and phantom <2
`grunt jasmine_node` tests *are* using jasmine 1.x but not phantom

Since the phantomjs (`jasmine`) tests weren't running on node 6,
I had to upgrade the library to get phantom to run at all.
This meant that we were then using Jasmine 2.x.
Accordingly, I updated all of the tests in spec/client to the new
syntax and updated some of the dependencies and the helper files.

At the same time, because the `jasmine_node` tests continued to
run successfully, I never changed them. Don't fix what ain't broke.

So now the `jasmine_node` tests are all still working, but they're
running jasmine 1.x, which means they need some of the old helper
files.
By contrast, the `jasmine` tests are all newly working, but
they're running jasmine 2.x and they need the updated helper files.

This means I've prefixed all the helper setup files with either
`client-`, `server-`, or `shared-` and updated the jasmine
configuration in the Gruntfile for both sets of tests to only
look for the right set of helper files.

//

Another change is that, before, the tests in the `/spec/shared/`
folder would be run by both the `jasmine` and the `jasmine_node`
tests. Since they are all written for Jasmine 1.x, they no
longer run in `jasmine` test run, and I'm just going to leave it
that way.
Return phantomjs jasmine tests to grunt unit tests
Back in 300c8ea, I took out some
broken tests so that we would have a deployable project.

I've reenabled the `jshint` command, and now I can also put back
the `jasmine` command as well.
Use PaaS stagecraft URL
The game has changed yo
window.history.replaceState was unspiable
Whether it's a new jasmine thing or whatever, it seems like it's
no longer possible to overwrite a funtion on the native `window`
object.

Source: https://stackoverflow.com/questions/8919370/jasmine-mock-window-object

So, as a (probably not ideal, but I've kind of had it with these
tests) workaround, we can just check the window.location.search
property directly and see that it is equal to the string we
expect.

In order that the `.location` doesn't become cluttered with other
parameters, we're truncating query parameters in the beforeEach
function.
This was also a problem for us in another file, but truncating
the query parameters there works equally well.
Streamline `jasmine` specs in gruntfile
Since we are including every file except for one, it's cleaner and
less ambigious to just use a global file matching pattern instead
of listing out the various directories.

Also, I'm stopping the SpecRunner.html from being saved by default
because it was useful when debugging but probably not actually
needed outside of that context.

@leelongmore leelongmore merged commit 13fe55c into master Oct 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@leelongmore leelongmore deleted the node-6-jasmine branch Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment