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

Test Helper Changes #7

Merged
merged 6 commits into from
Jan 12, 2016
Merged

Test Helper Changes #7

merged 6 commits into from
Jan 12, 2016

Conversation

zspecza
Copy link
Contributor

@zspecza zspecza commented Jan 11, 2016

Summary of Changes

  • Updated dependencies
  • Previously skipped tests now run on Travis
  • Disabled Roots.analytics for test helpers
  • You can now pass { async: true } as the last parameter to every test helper method. This makes the method run asynchronously and return a promise. This however, does not apply to project.compile and project.install_dependencies, which both are now fully asynchronous by default, as opposed to a mix of sync and async logic
  • Even though there is a breaking change, dependant code will not suffer from the change.

Breaking Change

  • disable analytics for project.compile method
    • why? because it doesn't make sense for test helpers and causes inconsistent failures for roots-contentful on Travis
    • /cc @Jenius @kylemac I need a review on this. I wasn't sure if I should add an option to Roots that allows disabling analytics at the instance-level so it doesn't override global config (e.g. new Roots(analytics: false) but I figured this would be okay since it is for testing only. It only becomes an issue if two instances of Roots are running at the same time on the same machine and one of them is running tests, which I figured is super edge-case, also - any recommendations on how to test this piece of logic?

note: even though this is a breaking change, it theoretically should not intefere with any dependent code. It simply removes an unwanted side effect of RootsUtil.Helpers. In other words, dependendant libraries need not worry about updating anything other than the version for roots-util as if there were no breaking changes.

Non-Breaking Changes

  • swap out deprecated fs.exists methods for one that uses fs.stat instead
  • add { async: true } option to helper methods to optionally make them asynchronous (they will return promises if this option is set)
    • why? because we now use AVA (which is concurrent) in some projects and having an async option for these helpers will help to improve test performance
  • stability: make is_empty tests run on CI, teardown, etc

@zspecza zspecza changed the title [WIP] Test Helper Changes Test Helper Changes Jan 11, 2016
W.all([nodefn.call(fs.readdir, dir), nodefn.call(fs.readdir, expected)])
.spread (dir, expected) -> String(dir) == String(expected)
else
String(fs.readdirSync(dir)) == String(fs.readdirSync(expected))
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering you're creating type String, you should just use the === operator as it won't perform type conversions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

coffee-script does this automatically: http://coffeescript.org/#try:alert%20'500'%20%3D%3D%20500

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, didn't realize it interchanged == and ===, been looking at javascript for too long hah

@hhsnopek
Copy link
Contributor

Looks good to me, you cleared up the only thought I had on the matter :)

@jescalan
Copy link

This is great, an excellent set of changes, well executed, and nice clean commits. On board with all the changes here as well. Gold PR star 🌟

I'll have this shipped out in a moment

jescalan pushed a commit that referenced this pull request Jan 12, 2016
@jescalan jescalan merged commit 1a56629 into master Jan 12, 2016
@jescalan jescalan deleted the analytics branch January 12, 2016 05:39
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.

3 participants