Skip to content

Extract tests out of queue into its own testQueue #1260

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.

Sign up for GitHub

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 8 commits into from
Mar 1, 2018
Merged

Extract tests out of queue into its own testQueue #1260

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a51ab3a
Extract tests out of queue into its own testQueue
step2yeung Feb 1, 2018
a088aa8
Removing TODO
step2yeung Feb 1, 2018
1f3e0ea
Simplifying addToTaskQueue to push ...tasksArray rather than use a fo…
step2yeung Feb 1, 2018
4e3f8b8
Test: Check length of queue and testQueue for after hook
step2yeung Feb 7, 2018
bf73554
Test: Adjust the after hook logic to be easier to reason
step2yeung Feb 7, 2018
a02b7a4
Core: Making taskQueue private to ProcessingQueue
step2yeung Feb 9, 2018
0fec182
Core: Adding comments around implementation
step2yeung Feb 21, 2018
c2000f4
Core:Fixing eslint errors
step2yeung Feb 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions bin/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ run.restart = function( args ) {

watchedFiles.forEach( file => delete require.cache[ path.resolve( file ) ] );

if ( QUnit.config.queue.length ) {
if ( QUnit.config.testQueue.length ) {
console.log( "Finishing current test and restarting..." );
} else {
console.log( "Restarting..." );
Expand All @@ -129,9 +129,9 @@ run.abort = function( callback ) {
}
}

if ( QUnit.config.queue.length ) {
const nextTestIndex = QUnit.config.queue.findIndex( fn => fn.name === "runTest" );
QUnit.config.queue.splice( nextTestIndex );
if ( QUnit.config.testQueue.length ) {
const nextTestIndex = QUnit.config.testQueue.findIndex( fn => fn.name === "runTest" );
QUnit.config.testQueue.splice( nextTestIndex );
QUnit.on( "runEnd", clearQUnit );
} else {
clearQUnit();
Expand Down
2 changes: 1 addition & 1 deletion reporter/html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export function escapeText( s ) {
abortButton.disabled = true;
abortButton.innerHTML = "Aborting...";
}
QUnit.config.queue.length = 0;
QUnit.config.testQueue.length = 0;
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion src/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ module.only = function() {
}

config.modules.length = 0;
config.queue.length = 0;
config.testQueue.length = 0;

module( ...arguments );

Expand Down
3 changes: 3 additions & 0 deletions src/core/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import { extend } from "./utilities";
const config = {

// The queue of tests to run
testQueue: [],

// The queue of tasks to run
queue: [],

// Block until document ready
Expand Down
74 changes: 44 additions & 30 deletions src/core/processing-queue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import config from "./config";
import {
defined,
generateHash,
now,
objectType
now
} from "./utilities";
import {
runLoggingCallbacks
Expand All @@ -23,67 +22,83 @@ let priorityCount = 0;
let unitSampler;

/**
* Advances the ProcessingQueue to the next item if it is ready.
* @param {Boolean} last
* Advances the taskQueue to the next task. If the taskQueue is empty,
* process the testQueue
*/
function advance() {
advanceTaskQueue();

if ( !config.queue.length ) {
advanceTestQueue();
}
}

/**
* Advances the taskQueue to the next task if it is ready and not empty.
*/
function advanceTaskQueue() {
const start = now();
config.depth = ( config.depth || 0 ) + 1;

while ( config.queue.length && !config.blocking ) {
const elapsedTime = now() - start;

if ( !defined.setTimeout || config.updateRate <= 0 || elapsedTime < config.updateRate ) {
if ( priorityCount > 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this code unneeded? What was its original purpose? Can you explain why we don't need it now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the original code, priorityCount's main purpose is to prioritize tests that was flagged to be executed first. The priorityCount was also incremented/decremented in advance() originally because if a tests needed to be inserted to the queue, it will be inserted after the tasks(which gets added to front of the queue) and after the already prioritized tests.
This is not necessary anymore since the task is in it's own queue, so we do not need to deal with the priortyCount when dealing with the tasks. (Only needed for the testQueue in the new implementation)

priorityCount--;
Copy link
Member

Choose a reason for hiding this comment

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

I believe we need to reintroduce this priorityCount decrement somewhere so that if a high priority test is added after the run starts, it should be placed in the right spot. I doubt we have a proper test for this, so we could always add that back with a test in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a TODO for that.

}

config.queue.shift()();
const task = config.queue.shift();
task();
} else {
setTimeout( advance );
break;
}
}

config.depth--;
}

if ( !config.blocking && !config.queue.length && config.depth === 0 ) {
/**
* Advance the testQueue to the next test to process. Call done() if testQueue completes.
*/
function advanceTestQueue() {
if ( !config.blocking && !config.testQueue.length && config.depth === 0 ) {
done();
return;
}
}

function addToQueueImmediate( callback ) {
if ( objectType( callback ) === "array" ) {
while ( callback.length ) {
addToQueueImmediate( callback.pop() );
}
const testTasks = config.testQueue.shift();
addToTaskQueue( testTasks() );
advance();
}

return;
/**
* Enqueue the tasks for a test into the task queue.
* @param {Array} tasksArray
*/
function addToTaskQueue( tasksArray ) {
for ( let i = 0; i < tasksArray.length; i++ ) {
const taskItem = tasksArray[ i ];
config.queue.push( taskItem );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this whole function can be simplified to:

config.queue.push(...tasksArray)

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it certainly can. Will update that.

}

config.queue.unshift( callback );
priorityCount++;
}

/**
* Adds a function to the ProcessingQueue for execution.
* @param {Function|Array} callback
* @param {Boolean} priority
* Adds a tests to the TestQueue for execution.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should be singular, test.

* @param {Array} testTasksArray
Copy link
Member

Choose a reason for hiding this comment

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

I could be wrong, but isn't this always going to be a function now? We pass the runTest function into here, and then later shift them off and then pass the expanded array to addToTaskQueue, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct! I renamed this to testTasksFunc

* @param {Boolean} prioritize
* @param {String} seed
*/
function addToQueue( callback, prioritize, seed ) {
function addToTestQueue( testTasksArray, prioritize, seed ) {
if ( prioritize ) {
config.queue.splice( priorityCount++, 0, callback );
config.testQueue.splice( priorityCount++, 0, testTasksArray );
} else if ( seed ) {
if ( !unitSampler ) {
unitSampler = unitSamplerGenerator( seed );
}

// Insert into a random position after all prioritized items
const index = Math.floor( unitSampler() * ( config.queue.length - priorityCount + 1 ) );
config.queue.splice( priorityCount + index, 0, callback );
const index = Math.floor( unitSampler() * ( config.testQueue.length - priorityCount + 1 ) );
config.testQueue.splice( priorityCount + index, 0, testTasksArray );
} else {
config.queue.push( callback );
config.testQueue.push( testTasksArray );
}
}

Expand Down Expand Up @@ -143,8 +158,7 @@ function done() {

const ProcessingQueue = {
finished: false,
add: addToQueue,
addImmediate: addToQueueImmediate,
add: addToTestQueue,
advance
};

Expand Down
18 changes: 8 additions & 10 deletions src/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ Test.prototype = {

if ( hookName === "after" &&
hookOwner.unskippedTestsRun !== numberOfUnskippedTests( hookOwner ) - 1 &&
config.queue.length > 2 ) {
config.queue.length >= 2 ) {
return;
}

Expand Down Expand Up @@ -367,27 +367,25 @@ Test.prototype = {
}

function runTest() {

// Each of these can by async
ProcessingQueue.addImmediate( [
return [
function() {
test.before();
},

test.hooks( "before" ),
...test.hooks( "before" ),

function() {
test.preserveTestEnvironment();
},

test.hooks( "beforeEach" ),
...test.hooks( "beforeEach" ),

function() {
test.run();
},

test.hooks( "afterEach" ).reverse(),
test.hooks( "after" ).reverse(),
...test.hooks( "afterEach" ).reverse(),
...test.hooks( "after" ).reverse(),

function() {
test.after();
Expand All @@ -396,7 +394,7 @@ Test.prototype = {
function() {
test.finish();
}
] );
];
}

const previousFailCount = config.storage &&
Expand Down Expand Up @@ -694,7 +692,7 @@ export function only( testName, callback ) {
return;
}

config.queue.length = 0;
config.testQueue.length = 0;
focused = true;

const newTest = new Test( {
Expand Down