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

chore: decaffinate more test scripts #1070

Merged
merged 1 commit into from Jul 16, 2018

Conversation

realityking
Copy link
Contributor

πŸš€ Why this change?

Removing some more cofffeescript bits & pieces.

Main advantage is that eslint lints these new JS files while nothing was linting the coffeescript files.

πŸ“ Related issues and Pull Requests

This completes the work started in #992

βœ… What didn't I forget?

  • To write docs
  • To write tests
  • To put Conventional Changelog prefixes in front of all my commits and run npm run lint

@realityking realityking changed the title chore: decaffinate more test scripts chore: decaffinate remaining test scripts Jun 20, 2018
@realityking
Copy link
Contributor Author

Why would it work on Linux but fail on Windows? πŸ€”

@honzajavorek
Copy link
Contributor

I didnt see the failing test (on mobile phone), but let me guess it’s the flaky beast we are trying to tame for second time in #1069

@realityking
Copy link
Contributor Author

I don't think that's it. There are actually 32 failures, by the looks of it in places I've touched in this PR.

@michalholasek
Copy link
Contributor

I wonder if coffee is doing file path delimiter normalization (\ vs. /). Might be worth it to try path.normalize or path.join.

@honzajavorek
Copy link
Contributor

Hmm, interesting. Looking at the failures I can't exactly say what could happen. The code seems to be the same. I suspected some implicit returns missing, I couldn't spot something right away tho, or Windows not being able to find node in PATH. But from the last error it's obvious the server process got started, so node is fine.

@realityking
Copy link
Contributor Author

It looks like most of the failures (all but one) are related in child-process-test.js which invokes the scripts differently (it's the only using spawn). I'll back out the things that are failing and make another PR for those.

@realityking realityking force-pushed the less-coffeescript branch 2 times, most recently from 2e2aea9 to 1438707 Compare June 28, 2018 16:53
@realityking realityking force-pushed the less-coffeescript branch 4 times, most recently from ffcffc5 to 35700b6 Compare July 8, 2018 17:24
@realityking realityking changed the title chore: decaffinate remaining test scripts chore: decaffinate more test scripts Jul 8, 2018
@realityking
Copy link
Contributor Author

A lot smaller in scope but at least the tests pass for what's in here :)

@michalholasek
Copy link
Contributor

@realityking I am sorry for the delay, we've had quite a lot of other work on our plates. Thank you for the PR, it's great!

@michalholasek michalholasek merged commit 4c97a87 into apiaryio:master Jul 16, 2018
@realityking
Copy link
Contributor Author

@michalholasek no worries, I know what it's like πŸ™‚

@realityking realityking deleted the less-coffeescript branch July 16, 2018 09:10
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.

None yet

3 participants