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

bin: add citgm-all #29

Merged
merged 9 commits into from
Dec 17, 2015
Merged

bin: add citgm-all #29

merged 9 commits into from
Dec 17, 2015

Conversation

MylesBorins
Copy link
Contributor

This PR adds a new bin to the project citgm-all

By default citgm-all will run the test suite of every package in the lookup table. If a custom lookup table is provided it will iterate across that instead.

TODO:

@MylesBorins
Copy link
Contributor Author

Express failed:

Looks like the test suite for 4.13.3 fails on the current stable node (5.1)
Master passes

Chalk failed:

Test suite is only failing inside of citgm. It looks like the color data that chalk is appending the strings is getting lost at some point. I assume this is because it is running in a child process. Will investigate #28

the project npm-run-all implemented this fix to keep colors on internal processes

mysticatea/npm-run-all@736dee0

It look like there needs to be a refactor of how we handle spawning processes, and how we spawn npm commands. There appears to be quite a bit of overlap between npm-run-all and citgm, perhaps we can borrow some from that project (license is MIT)

Colors failed

See above

bluebird failed

TypeError: process.stdout.cursorTo is not a function

Looks like some more weirdness with stdout, likely making assumptions that it is running top level

yeoman-generator

I think we likely want to be tracking yo, which is the cli tool, rather than yeoman-generator. The tests are failing regardless.

Test suite on Master fails too. Looks like we found a breaking change :D

redis fails

don't have redis setup 📦

jquery fails
the test suite for jquery 2.1.4 is failing on node 5.1. Master passes

handlebars fails

v4.0.5
Fatal error: 1 tests failed
(node) sys is deprecated. Use util instead.

master
Error: ENOENT: no such file or directory, scandir '/Users/thealphanerd/code/handlebars.js/spec/mustache/specs/'

Looks like handlebars is not doing so well right now :(

yosay

same problem as chalk

mime

AssertionError: 'audio/mp4' == 'audio/x-m4a'

Does not look related to node

Myles Borins added 3 commits December 15, 2015 15:49
Currently citgm breaks on url's that point to ssh://git@.

No longer will this be the case
@MylesBorins
Copy link
Contributor Author

/cc @jasnell I think this is ready to go

@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

Rubber stamp LGTM. Land it! /cc @rvagg

@MylesBorins MylesBorins merged commit a038403 into nodejs:master Dec 17, 2015
@rvagg
Copy link
Member

rvagg commented Dec 17, 2015

This is really good work @thealphanerd, I'm trying out citgm-all now and it's a huge improvement

@MylesBorins
Copy link
Contributor Author

Thanks @rvagg

I feel like this pass got citgm to the basically functionality it needs to have to be useful. On the next pass I'm going to start tearing at the guts and potentially make it more extensible. There is a TON of logic just for shelling out and managing npm... which is code I've written multiple times myself.

There is definitely a need for a userland module to do this, and I might try to abstract it out.

I'd also like to do a big overhaul of how state is managed, specifically trying to make the functions a bit more pure with less bleeding. Some things obviously need to be global / singleton like the logger... but I think we can do away with context all together and have the various pieces of the pipeline hand off objects with only the necessary data.

I'd also LOVE to work on some sort of caching layer to avoid the constant npm install, which is one of the largest bottlenecks for local use. That being said I'm not convinced this is as necessary as the above mentioned fixes.

I'll go ahead and make issues about the improvements I'd like to make so we are all on the same page before I go heads down

@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

+1 this is great work. Thank you for picking it up and running with it.
On Dec 16, 2015 5:53 PM, "Myles Borins" notifications@github.com wrote:

Thanks @rvagg https://github.com/rvagg

I feel like this pass got citgm to the basically functionality it needs to
have to be useful. On the next pass I'm going to start tearing at the guts
and potentially make it more extensible. There is a TON of logic just for
shelling out and managing npm... which is code I've written multiple times
myself.

There is definitely a need for a userland module to do this, and I might
try to abstract it out.

I'd also like to do a big overhaul of how state is managed, specifically
trying to make the functions a bit more pure with less bleeding. Some
things obviously need to be global / singleton like the logger... but I
think we can do away with context all together and have the various pieces
of the pipeline hand off objects with only the necessary data.

I'd also LOVE to work on some sort of caching layer to avoid the constant
npm install, which is one of the largest bottlenecks for local use. That
being said I'm not convinced this is as necessary as the above mentioned
fixes.

I'll go ahead and make issues about the improvements I'd like to make so
we are all on the same page before I go heads down


Reply to this email directly or view it on GitHub
#29 (comment).

@MylesBorins MylesBorins deleted the citgm-all branch December 17, 2015 23:25
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