Closed jshreyans closed 4 years ago
Also, I don't think there's any need to make a new test task as you suggested in #203. A script for that already exists in package.json
:
https://github.com/intermine/im-tables/blob/c2bced18e62b09c4bb468dc7eb050ec175e8e627/package.json#L13
So should I remove the test command from watch too and make it a separate task in grunt.initconfig?
Go ahead with restructuring the Grunt tasks as you see fit. A different approach would be to rename the current watch
to watch-tests
, then create a new watch task which is similar, except without the testing. I'm not sure which is best, so you'll just have to try for yourself.
@uosl I went forward with your idea to make a separate script to watch and run tests. I've modified the standard watch
grunt task and it will no longer run tests now. Tested the output and the npm run dev
command ran without errors. Hope this is fine!
Also, npm run dev
won't run the run:bundle_test_indices
task anymore.
@uosl I'll need some time to figure this out. I checked the grunt-contrib-watch
documentation and it does seem to have the events
option that could suit our needs. I'll see what I can do.
@uosl I wanted to get some clarifications regarding this. What we are looking for right now is a way to somehow test any file that the user modifies when running the project in watch
mode. So now that we're sure that the watchTests
script won't work, we'd want the user to just run npm run dev
such that it doesn't trigger any tests on its own, but editing files does trigger tests. Is that right?
No, we want to keep the current (new) behaviour of npm run dev
, meaning it should not trigger test runs.
What we want is a new command which will have the behaviour of the old npm run dev
, meaning it should run the tests whenever a test file or source file is changed. The watchTests
is supposed to do this, but sadly doesn't due to configuration being incompatible with grunt-contrib-watch
.
You could be picky with watch tasks (ie. make watchTests use watch:coffee_test, watch:test, etc., while serve uses the ones that you defined for watch) although then we would need a Grunt concurrent task runner to make sure they're running at the same time instead of sequentially.
I think this could be a way to make watchTests
work. We'd need to add grunt-concurrent
and something like this to the Gruntfile config. https://stackoverflow.com/questions/17585385/how-to-run-two-grunt-watch-tasks-simultaneously#17613210
I think this could be a way to make watchTests work. We'd need to add grunt-concurrent and something like this to the Gruntfile config. https://stackoverflow.com/questions/17585385/how-to-run-two-grunt-watch-tasks-simultaneously#17613210
This looks promising. I'll try it out asap.
@uosl I've abstained from committing to this branch for now to avoid any weird problems. I have made changes to the Grunt configuration and pushed changes to the test-grunt branch. Could you review these changes? Once they're approved, I'll simply merge that branch with this one.
These are the changes that I've made:
serve
grunt task is now selectively running the build tasks for the project. It has been tested and runs perfectlywatchTests
script has been modified to selectively run the testing tasks defined in watch
. These test tasks are defined in the watch
script itself. I've just defined the additional coffee_test
.package.json
, the start
script has been modified to run the npm run dev
and watchTests
tasks concurrently using grunt-concurrent
as you suggested. For now, running npm run dev
will do the same as it was doing before i.e. build the project and watch for changes without tests. The npm run start
script will do the same but with tests (hopefully :P)
I ran npm run start
and it did log that it was concurrently running tasks but the rest of the console output wasn't there. The script seems to have worked the files in /dist
were uglified and localhost:9000
was running (although flymine gives a CORS error for now). Waiting for your feedback!
I tried out your test-grunt
branch and it works great! You can go ahead and merge it into this PR.
There are two options I'd like to see added to the concurrent task: logConcurrentOutput set to true and indent set to false. I found this produces the most useful logging.
You should also add grunt-concurrent
as a dev dependency, as I had to install it manually.
I've made changes and merged test-grunt
into this PR. Should be reading for merging into dev
now. It seems we can finally complete the work from #206 too
@uosl no problems! After all, seeing your name among the project's contributors is a pleasure worth working for.
@uosl I've removed the
test
command from the build script in the Gruntfile. But there's another issue here. Thenpm run dev
script runsgrunt serve
which in turn runs 3 tasks: https://github.com/intermine/im-tables/blob/c2bced18e62b09c4bb468dc7eb050ec175e8e627/Gruntfile.js#L214-L218Now build will no longer run tests but
watch
will. In fact,watch
contains the definition of the test task. So should I remove the test command from watch too and make it a separate task ingrunt.initconfig
?