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

Update CI for Node v7 #99

Closed
2 tasks done
nfischer opened this issue Oct 28, 2016 · 9 comments
Closed
2 tasks done

Update CI for Node v7 #99

nfischer opened this issue Oct 28, 2016 · 9 comments
Assignees

Comments

@nfischer
Copy link
Collaborator

nfischer commented Oct 28, 2016

This should be done on both:

  • Travis CI
  • Appveyor

This should be a very easy change, and it's pretty low-priority. I can knock this out next week, but if anyone else would like to handle it, I can review. I'm self-assigning so I don't forget about it, but ping this thread if you'd like to take it on.

@nfischer nfischer self-assigned this Oct 28, 2016
@RichyHBM
Copy link
Contributor

I'd be happy to do this, should just be a couple of line additions. However Travis CI doesn't list Node 7 as a supported version yet! https://docs.travis-ci.com/user/languages/javascript-with-nodejs/

@nfischer
Copy link
Collaborator Author

@RichyHBM a PR would be welcome!

I believe Travis just uses nvm to manage node versions. A PR should prove it one way or the other.

@RichyHBM
Copy link
Contributor

RichyHBM commented Nov 11, 2016

Added Node v7 for both of these, which has inded made the tests run on node 7 however the tests are currently failing on travis due to new lines issues? I have run the tests locally and they all pass fine

Pull requests: #100 and #101

Both of these CI's also allow specifying a "latest" version, it may be useful to add that to the configs as well so that the project always gets tested with the latest version of node without having to specify it in the config, what do you think @nfischer ?

@nfischer
Copy link
Collaborator Author

I'm still confused about why the failures are occurring. Perhaps there's a change in Travis? Or perhaps a change in one of our dependencies? We'll have to investigate this before merging.

I don't see a benefit to adding "latest" in the CI. We want to test on node v4+ anyway, so for each new release we may as well add the latest node version. I've run into issues where tests have failed on newer node versions and required some revision. It would've been hard to debug this if the tests started failing at the same time as a new PR, only because node itself changed something.

@RichyHBM
Copy link
Contributor

Yup, very strange indeed, as mentioned in the PR are you able to make rtavis run a test on the master branch? Or re-run these tests?

I can go through the test files and change them to explicitelly use os.EOL instead of \n if you think that may help?

@nfischer
Copy link
Collaborator Author

@RichyHBM I restarted the travis jobs in your PRs. Let's see if this was just a hiccup with Travis.

If Travis still has issues, I'll try a fresh clone on my dev machine. My best guess is that some dependency updated with a breaking change without us realizing. Our code has been untouched for months, so I doubt it's a change on our end.

@nfischer
Copy link
Collaborator Author

So after looking at it a bit, it looks like something changed with how we detect the screen width. A lot of the failures have to do with ls, which detect terminal width to figure out when to wrap its output (with \ns) instead of spacing things out like default (with two spaces).

This isn't a bug with the source code (confirmed on my linux box), so it sounds like something we should mock during tests.

It also looks like there may be other bugs with the tests, but this should be the root cause of a few of the failures.

@nfischer
Copy link
Collaborator Author

@RichyHBM See #102. PRs are welcome. I'm confident that the approach I've outlined will fix all test errors.

Let's wait on fixing this issue until that one is resolved.

@nfischer
Copy link
Collaborator Author

Fixed by #100 and #101

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

No branches or pull requests

2 participants