Extract tests out of queue into its own testQueue by step2yeung · Pull Request #1260 · qunitjs/qunit

step2yeung · 2018-02-01T23:24:16Z

This PR separates the content of Qunit.config.queue which contains both tests (runTest function) and its tasks(each function added to the queue by runTest) into two separate queue: queue and testQueue.

Previously the queue was serving two purposes:

tracking the list of tests to run
when a given test is ran it unshifts that specific tests tasks to the front of the queue

This PR also goes along the lines of the first comment by platinumazure in this PR: #1124
The idea of this change is to clean up and simplify the queue logic to make it easier to reason about. The change separates the processing of tests and its tasks, giving a clear indication when all the tasks of a tests are complete, and when the next test can begin.

Before:

QUnit.config.queue = [
  function() { test.before() },
  function runHook(), # before
  function() { test.preserveTestEnvironment(); },
  function runHook(), #beforeEach
  function() { test.run(); },
  function runHook(), #afterEach
  function runHook(), # after
  function() { test.after(); },
  function() { test.finish(); },
  function runTest(),
  function runTest(),
  function runTest(),
  # ...
];

After:

QUnit.config.queue = [
  function() { test.before() },
  function runHook(), # before
  function() { test.preserveTestEnvironment(); },
  function runHook(), #beforeEach
  function() { test.run(); },
  function runHook(), #afterEach
  function runHook(), # after
  function() { test.after(); },
  function() { test.finish(); },
];

QUnit.config.testQueue = [
  function runTest(),
  function runTest(),
  function runTest(),
  # ...
];

jsf-clabot · 2018-02-01T23:24:17Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Stephen Yeung seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

rwjblue · 2018-02-01T23:30:47Z

src/core/processing-queue.js

 		const elapsedTime = now() - start;

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


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

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)

rwjblue · 2018-02-01T23:31:38Z

src/core/processing-queue.js

+function addToTaskQueue( tasksArray ) {
+	for ( let i = 0; i < tasksArray.length; i++ ) {
+		const taskItem = tasksArray[ i ];
+		config.queue.push( taskItem );


I wonder if this whole function can be simplified to:

config.queue.push(...tasksArray)

What do you think?

Yes, it certainly can. Will update that.

…r loop

trentmwillis

I haven't reviewed the changes yet, but I'm curious (as it isn't clear from the PR description) as to what the goal is with this change?

We're introducing another field into QUnit.config, but I'm not clear on what the benefit of doing so is for consumers. Is there a use case you have in mind for it?

rwjblue · 2018-02-09T04:34:30Z

I don’t think the intent is really related to exposing another queue, just trying to simply the queueing logic.

In the long run, having separate queues for the list of tasks for a single test and the pending tests will make it easier to add more logic around test to test advancing (e.g. so that measurements can be done to prevent memory leaks and/or identify per-test code coverage), but that is definitely not the goal of this PR itself. This PR is only about simplifying the queuing logic (having a single queue where some items are prepended and others are appended is really confusing).

rwjblue · 2018-02-09T04:36:13Z

If you’d prefer to avoid adding a new property on QUnit.config I think we can most likely do that.

@step2yeung - Can you confirm?

step2yeung · 2018-02-09T06:23:12Z

Thanks for clarifying @rwjblue!
We can avoid adding it to QUnit.config, and just have the testQueue be private to processing-queue.js.
If thats the case, then is it even necessary to expose the QUnit.config.queue?

trentmwillis · 2018-02-09T17:03:23Z

Yeah, my concern is primarily around adding a new public property. If we can keep testQueue internal, then I'm 👍 on this. We'll still need QUnit.config.queue because that is how the CLI/HTML Reporter abort/cancel test runs. We may want to add a proper public API for that behavior, but that should be a separate PR.

step2yeung · 2018-02-09T21:26:50Z

@trentmwillis Thanks for the feedback! I modified it such that config.queue contains the list of tests, where as taskQueue is now private to ProcessingQueue. This way, the understanding of QUnit.config.queue stays relatively the same when it comes to test run aborting/canceling, and we minimize the changes to two files.

rwjblue

This is looking really good to me, thanks for working on this cleanup @step2yeung!

@trentmwillis - Have any time for a follow-up review?

rwjblue · 2018-02-21T15:09:01Z

src/test.js

 			if ( hookName === "after" &&
 				hookOwner.unskippedTestsRun !== numberOfUnskippedTests( hookOwner ) - 1 &&
-				config.queue.length > 2 ) {
+				( config.queue.length > 0 || ProcessingQueue.taskCount() > 2 ) ) {


Not really related to this PR since it was pre-exixting, but this took me a little while to figure out (specifically, why is > 2 important), it may warrant an inline comment...

Agreed...should've done that last time I refactored this code

trentmwillis

Thanks for working on this! Sorry for being slow to review. Overall, the implementation seems pretty good. Mainly have some suggestions around comments/clarification for future travelers.

trentmwillis · 2018-02-21T21:06:25Z

src/core/processing-queue.js


 let priorityCount = 0;
 let unitSampler;
+const taskQueue = [];


It would be nice to have a comment describing what a task is within the taskQueue. Something to the effect of:

This is queue of functions that are tasks within a single test. After tests and removed from config.queue they are expanded into a set of tasks in this queue.

trentmwillis · 2018-02-21T21:09:01Z

src/core/processing-queue.js

- * Adds a function to the ProcessingQueue for execution.
- * @param {Function|Array} callback
- * @param {Boolean} priority
+ * Adds a tests to the TestQueue for execution.


Nit: Should be singular, test.

trentmwillis · 2018-02-21T21:11:21Z

src/test.js

 			if ( hookName === "after" &&
 				hookOwner.unskippedTestsRun !== numberOfUnskippedTests( hookOwner ) - 1 &&
-				config.queue.length > 2 ) {
+				( config.queue.length > 0 || ProcessingQueue.taskCount() > 2 ) ) {


Agreed...should've done that last time I refactored this code

trentmwillis · 2018-02-21T21:12:47Z

src/core/processing-queue.js

- * @param {Function|Array} callback
- * @param {Boolean} priority
+ * Adds a tests to the TestQueue for execution.
+ * @param {Array} testTasksArray


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?

Correct! I renamed this to testTasksFunc

trentmwillis · 2018-02-21T21:14:34Z

src/core/processing-queue.js


 		if ( !defined.setTimeout || config.updateRate <= 0 || elapsedTime < config.updateRate ) {
-			if ( priorityCount > 0 ) {
-				priorityCount--;


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.

Added a TODO for that.

trentmwillis

Looks good to me, thanks a bunch!

step2yeung added 2 commits February 1, 2018 11:43

Extract tests out of queue into its own testQueue

a51ab3a

Removing TODO

a088aa8

rwjblue reviewed Feb 1, 2018

View reviewed changes

step2yeung added 3 commits February 1, 2018 15:59

Simplifying addToTaskQueue to push ...tasksArray rather than use a fo…

1f3e0ea

…r loop

Test: Check length of queue and testQueue for after hook

4e3f8b8

Test: Adjust the after hook logic to be easier to reason

bf73554

trentmwillis reviewed Feb 9, 2018

View reviewed changes

Core: Making taskQueue private to ProcessingQueue

a02b7a4

rwjblue approved these changes Feb 21, 2018

View reviewed changes

trentmwillis requested changes Feb 21, 2018

View reviewed changes

step2yeung added 2 commits February 21, 2018 14:53

Core: Adding comments around implementation

0fec182

Core:Fixing eslint errors

c2000f4

trentmwillis approved these changes Mar 1, 2018

View reviewed changes

trentmwillis merged commit d5815d5 into qunitjs:master Mar 1, 2018

trentmwillis mentioned this pull request Jun 6, 2018

RangeError: Maximum call stack size exceeded #1284

Closed

raycohen mentioned this pull request Dec 12, 2019

QUnit 2.9 compatibility square/qunit-bdd#63

Merged

Conversation

step2yeung commented Feb 1, 2018

Uh oh!

jsf-clabot commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trentmwillis left a comment

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Feb 9, 2018

Uh oh!

rwjblue commented Feb 9, 2018

Uh oh!

step2yeung commented Feb 9, 2018

Uh oh!

trentmwillis commented Feb 9, 2018

Uh oh!

step2yeung commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trentmwillis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trentmwillis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

jsf-clabot commented Feb 1, 2018 •

edited

Loading

step2yeung commented Feb 9, 2018 •

edited

Loading