The Wayback Machine - https://web.archive.org/web/20201206230417/https://github.com/Autodesk/hubble/pull/61
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

Fixes #58: Adds a JS testing framework and other JS goodies #61

Merged
merged 8 commits into from Dec 14, 2017

Conversation

@filmaj
Copy link
Contributor

@filmaj filmaj commented Dec 13, 2017

I needed to make a few big changes to add JS testing. Additionally, this PR includes many tweaks to the documentation - so feedback is much appreciated 😄

Summary of Changes

  1. By far the biggest change is that I moved the JS dependencies we currently have (jquery, moment.js, Chart.js, d3.js) into this repo. I needed to do this so that the test runner is able to load them into our target JS environments (Chrome, in our case, but potentially we could add more in the future), since the main charts.js source file we author directly depends on them and assumes they are available globally.
    • If y'all don't like this change, I can look into seeing how viable mocking out all our JS dependencies would be. However, I expect this to be a tedious task, and if we ever add new JS dependencies (or update existing ones), we'd have to also update the mocks. 😒
  2. Tweaked CONTRIBUTING.md documentation to explain where to look for info on how to validate (i.e. run the tests) the web app component.
  3. Clarified the Getting Started section of the top-level README.md to explain what the updater and docs subdirectory / components are and linking to their relevant READMEs.
  4. Added a README.md to the docs/ component, explaining (in so far as I understand) how the web app works, how it is structured, and how to run tests (and in the hopefully-near-future, linter).
  5. Tweaked jekyll _config.yml to exclude the extra node.js bits, like dependency directory and config files.
  6. Added basic smoke tests for charts.js - nothing more than checking for the existence of our main createChart functions. Hopefully, with every bug we identify therein, or with every new JS feature we add, we can also add tests now.

Try It Out

Ensure you have Google Chrome installed (and updated). Ensure you have a recent version of node.js available. Pull this branch down, then:

$ cd docs
$ npm install # install dependencies
$ npm test # should exit with code 0 and print out friendly green text saying 4 tests passed

Want to see what a failing test looks like? Edit docs/spec/charts.js, and inside one of the test blocks (encapsulated via the it() function), add expect(false).toBe(true); - then rerun the tests via npm test. Oh no! Angry red text and error stack traces everywhere!

Copy link
Collaborator

@larsxschneider larsxschneider left a comment

Looks all good to me. Please take a look at my linter question... then I can merge 😉


### Linting

$ npm run lint

This comment has been minimized.

@larsxschneider

larsxschneider Dec 14, 2017
Collaborator

The linter complains it can't find a config file. I guess this is expected?

This comment has been minimized.

@filmaj

filmaj Dec 14, 2017
Author Contributor

Ah yes, I have it ready to go but I was going to wait until the tests were merged before adding all the various bits. I will remove these instructions for now until I put together the full additions for linting. I'll also rebase with latest master.

@larsxschneider
Copy link
Collaborator

@larsxschneider larsxschneider commented Dec 14, 2017

Plus, don't worry about the conflict. I can fix it while merging.

@filmaj filmaj force-pushed the filmaj:js-tests branch from 4ecfa92 to 0c2de75 Dec 14, 2017
@filmaj
Copy link
Contributor Author

@filmaj filmaj commented Dec 14, 2017

I've rebased and removed the linter instructions. Once this is merged, I've got all the parts for the linter ready to go - but I'll issue a separate PR for that.

@larsxschneider larsxschneider merged commit 1ad0910 into Autodesk:master Dec 14, 2017
@pluehne pluehne added the enhancement label Jan 8, 2018
@pluehne pluehne added this to To Do in Hubble Enterprise 0.2.0 via automation Jan 8, 2018
@pluehne pluehne moved this from To Do to Done in Hubble Enterprise 0.2.0 Jan 8, 2018
@pluehne pluehne added the testing label Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.