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

Build: Rearrange grunt/npm tasks into a build/dist/test pattern #1980

Closed
wants to merge 1 commit into from

Conversation

gibson042
Copy link
Member

@@ -55,6 +55,6 @@
"scripts": {
"build": "npm install && grunt",
"start": "grunt watch",
"test": "grunt test"
"test": "grunt && grunt test"
Copy link
Member

Choose a reason for hiding this comment

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

I've just noticed this will most likely not work on Windows unless run via Bash. This also affects current build script definition.

cc @dmethvin

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK then. I must have remembered sth incorrectly.

Copy link
Member

Choose a reason for hiding this comment

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

I use msysgit and bash on Windows, can't imagine getting work done without it. Even if our own stuff works I'd be concerned that something would be amiss in the 27-bazillion levels of npm dependencies below us. However, we pulled out the README instructions for installing msysgit a while back and nobody has come in to say they can't get things up and running on Windows. Not sure what that means, maybe I'm the last Windows user.

@dmethvin
Copy link
Member

👍

@mgol
Copy link
Member

mgol commented Dec 30, 2014

LGTM, too. If we ever want to make Node smoke testing optional, we can tweak the setup further.

@markelog didn't want to have this smoke testing run on every grunt execution so I'd like to hear his opinion as well. But it seems most is in favor of it.

@markelog
Copy link
Member

markelog commented Jan 3, 2015

So at grunt test we would not lint anything? But ci-run will execute those tests twice?

This:

grunt.registerTask( "test_fast", [ "node_smoke_test" ] );
... 
grunt.registerTask( "test", [ "test_fast" ] );

Is look like very weird, test is test_fast and test_fast is node_smoke_test, confusing, i assume, this is done for the future but it looks pretty incoherent to me.

Whereas this node_smoke_test just require stuff, not running qunit tests, i don't see the point doing this everytime for the default task, you will find more issues with your code with jshint task, not with this one, this smoke stuff could find the problem but in very rare cases. For me, it would be even okay, if we would run it before the release, certainly not everytime, but that's me.

I feel we redoing a lot and make arguably confusing changes for very small benefit. I'd create an npm command, that runs this jsdom require and execute it with ci-run or even without npm command and only with grunt task just like grunt testswarm

@gibson042
Copy link
Member Author

So at grunt test we would not lint anything?

Right; testing and linting are separate processes.

But ci-run will execute those tests twice?

The fast ones, yes. Or we could introduce a currently-empty test_slow (for eventually population by the full test suite) and use that to avoid duplication. The goal here is to set up a basic structure: lint (preceded by build for us because our source is deconstructed), output to dist (including minification/tree-shaking/etc.), test the result.

// Short list as a high frequency watch task
grunt.registerTask( "dev", [ "build:*:*", "lint" ] );

grunt.registerTask( "lint", [ "jsonlint", "jshint", "jscs" ] );
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to register it above the "dev" task

@markelog
Copy link
Member

markelog commented Jan 4, 2015

I think it would fine if have something to structure, now we only have this require and couple tasks that do this require, that do the same thing, "test_fast" name doesn't make sense, at least now.

@gibson042
Copy link
Member Author

"test_fast" name doesn't make sense, at least now.

As it turns out, #1821 or #1996 or similar will introduce such a need... node_smoke_test is fast, but promises-aplus-tests is slow.

@gibson042 gibson042 closed this in bb928bd Jan 11, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants