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

tests failing, also not all tests are running #92

Closed
hkasemir opened this issue Nov 14, 2017 · 7 comments
Closed

tests failing, also not all tests are running #92

hkasemir opened this issue Nov 14, 2017 · 7 comments
Assignees

Comments

@hkasemir
Copy link
Contributor

Following instructions to set up and test the fluent packages results in tests failing

also, this line is preventing some tests from running: https://github.com/projectfluent/fluent.js/blob/master/fluent/test/functions_builtin_test.js#L86

with that .only included, one test fails, by removing it, 6 tests fail.

@zbraniecki zbraniecki self-assigned this Nov 14, 2017
@zbraniecki
Copy link
Collaborator

The .only has been introduced in #81. With it, I see:

> make test
  59 passing (99ms)
  49 pending

when i remove it I see:

> make test
  196 passing (170ms)
  54 pending

@hkasemir - can you investigate which tests are failing and why? I suspect something is off with your node environment and its different than the one we have. It may be that you need Intl API, or it may be that we need to handle more variants of Node+Intl combinations, but I'd like to start with identifying tests and their failures.

@stasm
Copy link
Contributor

stasm commented Nov 15, 2017

Sorry about the regression introduced in #81. I use .only quite often to isolate a single test during development. I found https://www.npmjs.com/package/eslint-plugin-no-only-tests which would prevent such errors in the future.

@stasm
Copy link
Contributor

stasm commented Nov 15, 2017

There's also https://www.npmjs.com/package/eslint-plugin-mocha which has more configuration options.

@stasm
Copy link
Contributor

stasm commented Nov 15, 2017

I opened #94 to add a linting rule forbidding .only tests.

I have the same output as @zbraniecki before and after fixing that rogue .only in functions_builtin_test.js. I tried on a fresh clone and I was able to set it up by following the instructions in the README. I'm using node 8.9.1.

@hkasemir
Copy link
Contributor Author

using node v6.11.2 and npm 5.4.1 fresh clone of the repo:
image

using node v8.9.1, seems all tests are passing 🤔 could be a good idea to define which version of node is required in the readme under instructions for local dev. Glad that was an easy fix. I did take the liberty of checking #93 to make sure it was not fixed by node version, but it is still an issue.

@stasm
Copy link
Contributor

stasm commented Nov 16, 2017

I found out why the tests fail. Node 6.x doesn't support Object.entries which is used here:

for (const [name, opt] of Object.entries(opts)) {

Given that node 8.x has already become the active LTS version I think it makes sense to explicitly drop support for 6.x (and 7.x too, even though the tests actually pass on 7.x).

@stasm
Copy link
Contributor

stasm commented Nov 21, 2017

With #94 and #95 merged, this should be good to close. Thanks, @hkasemir, for the report!

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

No branches or pull requests

3 participants