Core: fix module.only logic for "flat" structures

[smcclure15]

Fixes #1272

I'm still a little worried about more edge cases, but I wanted to get this out there and am very open to feedback on the approach.

For every module after a module.only call, we need to say that the current module is then closed for appending, so that later tests can stay out of them. They are still a no-op for performance, but they need to communicate this extra piece.

[smcclure15]

This deleted file is causing a build failure (I see the 404's print in my local dev, but it didn't indicate a failure like Travis did).
It was being used in the "sandboxed-iframe" test, I think to try to isolate it as much as possible. That should be resolved now with the CLI test suite, and I think some or all of the "bridge" injections can go away now. I'm still grasping at the intent of that sandbox test, so I'm curious of any historical knowledge others can share.

[Krinkle]

Based on the commit history for these files, I believe its purpose is not so much testing of the QUnit.only functionality, but rather it is a higher-level integration test for running QUnit from inside an iframe. Specifically, a sandbox iframe that has limited capabilitities. Ref #1091.

As such, some form of this should remain. See #1091 (comment) for more information. The test needs to trigger code that tries to use QUnit.config.storage (aka localSessionStorage, aka window.sessionStorage).

[smcclure15]

I've restored this sandboxed-iframe test.
I brought back the only.js test logic, albeit renamed to sandboxed-iframe.js to not be confused with the only tests.
(also, I now see some whitespace changes in the HTMLs that I can correct right away)

It sounds like this might be able to be improved, but this is admittedly beyond my scope, so I'm restoring to status quo to keep issues independent.

[Krinkle]

Based on the commit history for these files, I believe its purpose is not so much testing of the QUnit.only functionality, but rather it is a higher-level integration test for running QUnit from inside an iframe. Specifically, a sandbox iframe that has limited capabilitities. Ref #1091.

As such, some form of this should remain. See #1091 (comment) for more information. The test needs to trigger code that tries to use QUnit.config.storage (aka localSessionStorage, aka window.sessionStorage).

[Krinkle]

It took me a while to realize that this is actually mutating the "previous" module. The one we are processing now, never makes it into the registry. This feels a bit fragile as it means test() calls are thus interacting with a closed module and I can see that leading to some confusing failures in the future.

On the other hand, given the way that only is meant to act like a filter, the module isn't meant to be visible in any way. Perhaps, though, it would be best to still let it enter the registry, but as an empty module that (if all goes well) will never start or stop, because it is empty. Afaik that is how empty modules are treated.

If empty modules are exposed to reporters or callbacks in some way, then we'd need some additional logic to make sure e.g. empty modules with currentModule.ignored get bypassed in our events/callback layer.

[Krinkle]

Let me know if you like that direction. I haven't thought it through fully, so it's not a strong suggestion, just an idea.

For now my only suggestion would be to add an inline comment indicating what we have done, which is to keep the previous module on the stack and that "closed" means it was implicitly closed by a subsequent module() call.

[smcclure15]

I'm certainly intrigued by that suggestion, and the notion of currentModule.ignored has a much more natural feel and intuition.

I think it's inside module.only where we set config.modules.length = 0; to effectively wipe out the others that were registered before. I'm trying to make that work with something like

config.modules.forEach( module => {
   module.ignored = true;
} );

but I'm definitely wrestling with the unittests - I don't have everything quite right.

I'll keep this open and continue noodling. I agree we should document the exposed property, and ignored is just so much easier to digest than closed (or alternative).

[Krinkle]

Nice 👍

I notice that when I modify the Plnkr test case in #1272 to use a scoped module, the issue does not happen. I guess that's because we just skip execution of the closure, and not actually due to any explicit logic making that happen.

For skip and todo, we currently inherit the state from inside the Test constructor, which is kind of interesting. Although that approach probably wouldn't work well here since only is really an extension of filter and not of skip (that is, when using only the other tests shouldn't be registered at all, not even skipped).

I like the approach this takes. Just two minor nit picks:

• The new property should be explicitly declared and set to false by default.
• The purpose of this property, and the new behaviour of keeping the previous module on the stack, is worth documenting.

[smcclure15]

Updated PR with ignored property instead of the misleading closed.
Some of the logic definitely simplified, and it feels more intuitive.

If that direction looks good, I'll move on to documenting that new property!

[Krinkle]

LGTM. Will try to land soon. Thanks!

[Krinkle]

Also confirmed the fix at https://plnkr.co/edit/bBpWs5X2vKp2YPTu (forked from original in #1272).