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

Core: Revamp processing order to enable proper aborting of test runs #1124

Merged
merged 2 commits into from
Mar 28, 2017

Conversation

trentmwillis
Copy link
Member

@trentmwillis trentmwillis commented Mar 24, 2017

Note: This is a follow up to #1121.

The Problem

Bear with me a moment, while I explain what this PR is doing. In the current system, when you add a test, it is queued up as a single function, named run. Thus, when a test suite begins running, the queue generally looks something like this (not real code, just for illustration purposes):

QUnit.config.queue = [
  function run(),
  function run(),
  # and so on, for however many tests you have
];

When we begin processing the queue, each of those individual run functions is replaced by a series of sub-functions. These functions, however, wind up at the back of the queue. Thus, after processing the first item you get:

QUnit.config.queue = [
  function run(),
  # ...
  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(); }
];

This is not intuitive. It also means that by the time we actually begin processing tests we have a massive queue of anonymous and runHook functions. This makes it very difficult to determine where one test ends and another begins by looking at the queue.

Previously, this knowledge was not very important, but with the coming CLI and specifically the --watch option, we need to be able to tell where tests start/end in the queue so that we can properly abort the test run while still allowing the test to cleanup after itself.

The Solution

So, what this PR does is change the processing order such that as each test is processed, it is added to the front of the queue so that it is processed immediately. In other words, the original queue above would become the following after processing the first test:

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 run(),
  # ...
];

This means that further tests will remain as a self-contained run function until the current test is finished executing.

This makes it easy to tell where the current test ends and the next one begins. It also means that the queue will remain relatively compact, instead of expanding every single test ahead of time.

This PR also revamps some of the behavior in the system that was previously, implicitly relying on the fact that all tests were expanded before processing any of them (specifically, how we handled hooks for tests). Overall, I think this makes the processing behavior much more understandable and less dynamic, while also enabling a better CLI experience.

Thanks for taking the time to read and review this!

@trentmwillis trentmwillis changed the title Processing order Core: Revamp processing order to enable proper aborting of test runs Mar 24, 2017
@trentmwillis trentmwillis mentioned this pull request Mar 24, 2017
1 task
@platinumazure
Copy link
Contributor

Haven't had a chance to review the code, so I apologize if any of my thoughts here are redundant, obviated, or just pointless.

Do we really need to have all tests' functions (even the basic run functions) in the processing queue at all? I feel like it might make more sense to have just one test's functions in the processing queue and maintain a separate queue of the test instances. Then the process queue would always look something like:

[
    runHooks("beforeEach"),
    runTest(),
    runHooks("afterEach"),
    getNextTest()
]

Where getNextTest() extracts the next tests from the queue and adds the appropriate functions to the queue. Starting a module would enqueue before hooks before dequeuing the first test. At the end of a module, of course, the after hooks would be queued. But basically, the processing queue would only need to track one test's worth of functions at most, and we can queue the tests as class instances instead of as functions.

@trentmwillis
Copy link
Member Author

@platinumazure I think that is a good long-term direction. In this PR I wanted to avoid changing the structure of QUnit.config.queue too much. Even though it is not really public API, it has never been documented explicitly as private and so I'm wary of changing the semantics of it too much.

I can't imagine anyone having gotten much use from the processed version of the queue, but I could see tools using the pre-processed version of the queue where you could count the total number of tests.

@platinumazure
Copy link
Contributor

@trentmwillis Okay, I can buy that my suggestion would be a breaking change and should be done with a major release. I'll give this a review tomorrow.

@trentmwillis trentmwillis merged commit 51e4382 into qunitjs:master Mar 28, 2017
@trentmwillis trentmwillis deleted the processing-order branch March 28, 2017 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants