Add basic tests #160
Add basic tests #160
Conversation
... because this is a better description of what this build target does and frees up the 'test' target to be used for running tests.
.travis.yml
Outdated
| @@ -12,6 +12,8 @@ before_install: | |||
|
|
|||
| install: | |||
| - yarn install --no-lockfile | |||
| - npm install openlayers@^3 --save-dev | |||
There was a problem hiding this comment.
we're using yarn in this project, please avoid npm here.
also, why --save-dev in the CI config instead of adding it as a dev dep in package.json?
There was a problem hiding this comment.
You're right, the --save-dev isn't necessary here; I'll remove it.
The use of yarn here had confused me, since it isn't in the development dependencies. Is there a reason for that? In this project it looks like it's only used in Travis (I'd even thought about proposing to replace it with just npm!). Should its use perhaps be documented in the README?
package.json
Outdated
| "test": "gulp lint" | ||
| "lint": "gulp lint", | ||
| "test": "jest", | ||
| "cover": "jest --coverage" |
There was a problem hiding this comment.
we can just call yarn test --coverage, so I don't think the extra cover script is necessary :)
There was a problem hiding this comment.
Ok, good to know. I'll remove the cover target.
There was a problem hiding this comment.
I've removed the cover target and have replaced npm with yarn.
There was a problem hiding this comment.
... although I've just found out that yarn can't install openlayers@^3. Odd that the installation works with npm though...
tests/sidebar-ol.test.js
Outdated
| expect(sidebar._options.element).toBe('sidebar'); | ||
| expect(sidebar._tabitems.length).toBe(0); | ||
| expect(sidebar._panes.length).toBe(0); | ||
| expect(sidebar._closeButtons.length).toBe(0); |
There was a problem hiding this comment.
these are testing implementation details of the Sidebar class. we should be testing the resulting HTML instead.
There was a problem hiding this comment.
I know, and this was something I wanted to get rid of over time. It wasn't very clear what a "finished" bare-bones Sidebar object actually looked like and in order to test this I had to dig into the internals (which I know is a dodgy thing, but it helped get this far). However you're right, these tests are effectively covered by later, more involved tests and hence can be removed.
There was a problem hiding this comment.
I've replaced these expect statements with a simply truthy check. Hope that's ok :-)
| var ol; | ||
| if (navigator.userAgent.includes("Node.js") || navigator.userAgent.includes("jsdom")) { | ||
| ol = require('openlayers'); | ||
| } |
There was a problem hiding this comment.
since this is needed only for tests I would prefer it if we could do this in the test file instead 🤔
There was a problem hiding this comment.
If I could have left the main code alone, I would have. I certainly tried very hard! Unfortunately, there seems to be no way to have the sidebar code "know" about OpenLayers (i.e. the ol namespace) at require time other than explicitly require-ing it inside the module itself. It didn't seem possible to load a <script> within the HTML loaded as part of jsdom in the jest setup, so it didn't seem to be possible to put ol into the global namespace for sidebar to find it. Do you have any ideas as to how one might be able to get around this?
These tests currently only work in OL3. Unfortunately I couldn't get the tests to work in OL4-6 yet but hope these can be added over time (there are issues with the `canvas` element in `node` that I've not yet worked out). The openlayers module is explicitly left out of the devDependencies because I think it's more likely that the continuous integration environment will install the relevant OL version and then run the test suite; hence it's not necessary to tie down developers with a particular OL version as part of development. I've chosen to use "jest" as the testing framework here since it seems to be the most popular testing framework currently and it's used by other OpenLayers-related projects such as ol-contextmenu. Also note that it was necessary to load the OpenLayers module explicitly within the sidebar-v2 module itself so that OpenLayers was available for the code running the tests. Since the loading code only runs if node/jsdom are available, it doesn't have an influence on the main of code which runs in the browser.
... at least ones which are different to the absolute bare-bones config.
paultcochrane
force-pushed
the
pr/add-basic-tests
branch
from
a133e49 to
12425f7
Compare
paultcochrane
force-pushed
the
pr/add-basic-tests
branch
from
12425f7 to
787b636
Compare
The patches in this PR add basic testing to the project. I chose to use
jestfor the testing framework because it seems to be very popular and it's also used by other OpenLayers projects (such as ol-contextmenu). At present, only OpenLayers v3 can be used to run the tests due tocanvaselement issues in Node with higher versions. I'm going to try to get OL versions 4-6 working, but that might take some time. Nevertheless, here's a test suite which exercises just over 80% of the code, so hopefully that helps out a bit!If you'd like anything changed with this PR, please don't hesitate to contact me and I'll update and resubmit as necessary.