Skip to content

Record start time of last started tasks #17

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

Merged
merged 3 commits into from
Mar 20, 2015

Conversation

dinoboff
Copy link
Contributor

The files used to compare to lastRun time might get updated during the execution of a task.

Instead of recording the stop time, Undertaker should record the start time, before something like gulp.src start to read the content of a file.

@dinoboff dinoboff force-pushed the dinoboff-record-start-time branch from 020c16b to e2c80a9 Compare March 16, 2015 19:31
The files used to compare to lastRun time might get updated during the execution of a task.

Instead of recording the stop time, Undertaker should record the start time, before something like gulp.src start to read the content of a file.
@dinoboff dinoboff force-pushed the dinoboff-record-start-time branch from e2c80a9 to 0ce3ed9 Compare March 16, 2015 19:35
expect(taker.lastRun('test1')).to.be.equal(since);

done();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That last test case in only meant to get 100% coverage (there must be a start event matching a stop or error event).

Maybe I should not test if _lastRuns[task] is set in the error and stop handler instead.

@dinoboff
Copy link
Contributor Author

Looking at the integration test that fails on io.js:

  • my implementation wouldn't work with that use case
  • I don't know why they are not failing on nodejs too

@dinoboff dinoboff force-pushed the dinoboff-record-start-time branch 6 times, most recently from 36fcc39 to 0ea2c40 Compare March 17, 2015 02:42
@phated
Copy link
Member

phated commented Mar 17, 2015

@dinoboff looks like you got it fixed, what was the problem?

@dinoboff dinoboff force-pushed the dinoboff-record-start-time branch from 0ea2c40 to f9e8d8a Compare March 17, 2015 12:05
@dinoboff dinoboff force-pushed the dinoboff-record-start-time branch from f9e8d8a to 3144540 Compare March 17, 2015 12:13
@dinoboff
Copy link
Contributor Author

@phated
First I had to change the test. The original was testing that the built file by the task was older than the last task run time. With the pull request, the last task run time is the start of the task; the built file should always be younger than the last run time. I didn't know why it was only failing on iojs.

I changed the test to check that if a source file changes, using the since option with lastRun, the unchanged source file would get filtered out. However the test needs to wait before modify a file to test it.

I think files' mtime and node time (or the time the extension before method records) are not consistent. On my laptop (os x 10.9, node 0.10) mtime seems to get round dow to the second:

task setup started at 2015-03-17T12:10:04.347Z
task build started at 2015-03-17T12:10:04.355Z
task userWait started at 2015-03-17T12:10:04.362Z
task userEdit started at 2015-03-17T12:10:05.484Z
task countEditedFiles started at 2015-03-17T12:10:05.486Z
counting /Users/dinoboff/dev/undertaker/test/fixtures/tmp/testMore.js
mtime 2015-03-17T12:10:05.000Z

Note that the file get modified after 12:10:05.484Z, but its mtime is 12:10:05.000Z. Seems be the same for travis node 0.10

On travis nodejs 0.12, it's not as bad, but it's still inconsistent:

task setup started at 2015-03-17T12:14:23.087Z
task build started at 2015-03-17T12:14:23.101Z
task userWait started at 2015-03-17T12:14:23.108Z
task userEdit started at 2015-03-17T12:14:24.209Z
task countEditedFiles started at 2015-03-17T12:14:24.211Z
counting /home/travis/build/phated/undertaker/test/fixtures/tmp/testMore.js
mtime 2015-03-17T12:14:24.208Z

userEdit Task starts at 12:14:24.209Z but the file mtime is 12:14:24.208Z.

On Travis iojs the task start time and the file mtime are the same.

@dinoboff
Copy link
Contributor Author

@phated

Reading around:

  • FAT32 time resolution is 2s
  • Ext3 and HFS: 1s.
  • NTFS: 100ms
  • Ext4: < 1ms
  • node fs stat time resolution is at most 1s in Node 0.10
  • node fs stat time resolution is at most 1ms in Node 0.11+

I don't know if the issue with node 0.12 (mtime < start task in that travis run) is due to a bug with node 0.12, with travis container or with the extension before method.

Should Undertaker round down the last run time for consistency?


The default time resolution is `1000` on node v0.10, `0` on node 0.11+ and iojs.
it can be overwritten using `UNDERTAKER_TIME_RESOLUTION` environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phated What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I dig it. I didn't know about these cases. Thanks!

@phated
Copy link
Member

phated commented Mar 20, 2015

@dinoboff this PR is awesome. thanks for all the work and research put into this

phated added a commit that referenced this pull request Mar 20, 2015
Record start time of last started tasks
@phated phated merged commit a00ef21 into gulpjs:master Mar 20, 2015
phated added a commit that referenced this pull request Jun 19, 2016
Record start time of last started tasks
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.

2 participants