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

Add cargo queue type and tests #1567

Merged
merged 5 commits into from Aug 7, 2018
Merged

Conversation

justinmchase
Copy link
Contributor

@justinmchase justinmchase commented Aug 1, 2018

I basically copied the code and tests from cargo. I then updated the function to expose the concurrency parameter and the comments to include copies from the queue code.

If there are docs in any other locations that need to be updated please let me know.

This fixes #1555

Copy link
Collaborator

@aearly aearly left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! A couple tweaks are needed though.

/**
* A cargoQueue of tasks for the worker function to complete. CargoQueue inherits all of
* the same methods and event callbacks as [`queue`]{@link module:ControlFlow.queue}.
* @typedef {Object} CargoObject
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to redefine the CargoObject, you can refer to the one defined in cargo.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rename it here to be @typedef {Object} CargoQueueObject ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The returned object has the same shape as the CargoObject, so there's no need for a duplicate JSDoc definition.

* @param {number} [payload=Infinity] - An optional `integer` for determining
* how many tasks should be processed per round; if omitted, the default is
* unlimited.
* @returns {module:ControlFlow.CargoQueueObject} A cargoQueue object to manage the tasks. Callbacks can
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, you can refer to CargoObject here.

var {expect} = require('chai');
var assert = require('assert');

describe.only('cargoQueue', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you left the .only() in!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops!

};
});

it('without callback', (done) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test failed in one of our CI runs. Can you think of a way to re-write it so that it's more robust? We've been finding that using setTimeout isn't very consistent in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't think setTimeout is actually deterministic, which can be a real pain for tests.

I thought about this for a while and it seems like the best solution would be a kind of elaborate recorder which would record the state of the queue at each worker invocation and callback. It would have to allow you to, instead of using time, use event frames. Using setImmediate is deterministic, the callbacks will be called in the order they were queued. Therefore your worker function and push callbacks could record the state of the queue and use setImmediate to move from frame to frame.

You could then have a way to describe which frames input is inserted and then have expectations set which are unordered. Every action would record the state of the queue in each frame.

This is total psuedo code but I was picturing something like this:

it('example', (done) => {
  var concurrency = 2
  var payload = 2
  var tester = new QueueTester(concurrency, payload) // contains cargoQueue
  tester.input([
    [1, 2],          // push at frame 0
    [3, 4, 5, 6]   // push at frame 1
  ])

  // For the above:
  // at frame 0 two items are enqueued
  // at frame 1 the first worker pops both items and processes them, 4 items are pushed
  // at frame 2 the first worker completes, the second worker pops 2 items and processes them
  // at frame 3 the first worker pops 2 items and processes them, the second worker completes
  // at frame 4 the first worker completes
  // at frame 5 the queue is idle and empty
  tester.expectation([
    { length: 2, workers: [] },
    {
      length: 0,
      workers: [
        { received: [1, 2] }
      ]
    },
    {
      length: 2,
      workers: [
        { completed: [1, 2] },
        { received: [3, 4] }
      ]
    },
    {
      length: 0,
      workers: [
        { received: [5, 6] },
        { completed: [3, 4] }
      ]
    },
    {
      length: 0,
      workers: [
        { completed: [5, 6]
      ]
    },
    {
      length: 0,
      workers: []
    }
  ])
})

tester.run(done)

I didn't go that far, instead I just altered the one failing test to use setImmedate instead of setTimeout and nested them so that they went in the same order as the original test. I think that's more deterministic but it sort of gross. Let me know if you had something else, more specific in mind.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I've started using a similar strategy for newer tests, verifying concurrency at various checkpoints, and verifying the correct order of execution. It will be a pain to retrofit all the older setTimeout based tests though.

@aearly aearly added the queue label Aug 2, 2018
setImmediate(() => {
c.push(3);
c.push(4);
setImmediate(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good enough for now, though.

@aearly aearly merged commit db49b89 into caolan:master Aug 7, 2018
@justinmchase justinmchase deleted the feature/cargoQueue branch August 14, 2018 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: add cargoQueue
2 participants