qunit-fixture has incorrect styles

[asapach]

Looks like my fix for #104 is incomplete. Currently #qunit-fixture is being assigned some inline styles: https://github.com/karma-runner/karma-qunit/blob/a431d3999b5136092a20e4c53babf4865ac78df6/src/adapter.js#L47-L55 But since we now set runner.config.fixture as an empty string: https://github.com/karma-runner/karma-qunit/blob/a431d3999b5136092a20e4c53babf4865ac78df6/src/adapter.js#L56-L58 QUnit will wipe out those styles, because it creates a new element instead of re-using the existing one: source

My initial suggestion was to save the element as runner.config.fixture = fixture, which fixes the issue, but as @Krinkle noted, the docs don't mention anything about it being a node.

Another potential fix would be to remove the inline styles entirely and rely on the QUnit CSS, which might be more future-proof. This means loading the CSS regardless of the showUI flag: https://github.com/karma-runner/karma-qunit/blob/a431d3999b5136092a20e4c53babf4865ac78df6/lib/index.js#L15-L17

What do you guys think?

Here's a repro: https://github.com/asapach/karma-qunit-fixture

[Johann-S]

I'm more in favor to add those style in the new fixture created by QUnit and I don't want to force users to use QUnit css if they doesn't want to

[asapach]

@Johann-S, OK, but that will partially undo #93 - we'll have to move part of the logic from begin() back into testStart(). And the remaining logic is kind of unclear:

• Do we still want to append the qunit-fixture element to the body even if we know that QUnit will wipe it out?
• If there's an existing element e.g. using customContextFile -- should we mess with it?

// CC @Krinkle

[Krinkle]

I have no strong opinion specifically on whether to load QUnit's stylesheet in karma-qunit.

However, I would prefer that we not preserve the fixture's DOM reference between tests. QUnit should be allowed to cleanly re-create the fixture for each test.

That leaves us with two options:

1. Try to re-assign the inline styles in a testStart hook.
2. Remove the need for inline styles and load the QUnit stylesheet.

Option 1 should work and is quick fix with minimal change.

Option 2 is a slightly bigger change, but I think we should consider it. For a few reasons:

• Currently we allow users of karma-qunit to control which version of qunit is used (via package dependencies).
• The reason we have these inline styles is because we want to match behaviour with what QUnit does. I imagine that for 99% of cases, the tests will pass even without these inline styles in karma-qunit, but we set them to ensure 100% parity between Karma and QUnit's web-based test runner.
• Right now, we achieve that by keeping a hardcoded copy of QUnit's styles in JavaScript ("post" / "fix-up"). This works, but requires maintenance from karma's side, and will at some point cause a problem when the styles change, unless we add more complexity by changing the styles based on QUnit.version.
• If we load QUnit stylesheet instead we get: Still matches QUnit's behaviour, and this time using the same mechanism. Does not require maintenance. ** Automatically in sync with the current QUnit version specified by the user.

@Johann-S wrote: I don't want to force users to use QUnit css if they doesn't want to

I would like to better understand your perspective. It seems both options are mechanisms internal to karma-qunit. In what way does it force or relate to users?

[asapach]

I've created a PR that illustrates what it would look like if we got rid of inline styles: #110

[Johann-S]

@Krinkle if we enforce the use of QUnit css, showUI option won't be an option, it will be the default behavior, it looks like a regression or a breaking change

[asapach]

@Johann-S, UI is not enabled until you've added #qunit element: https://github.com/karma-runner/karma-qunit/blob/a431d3999b5136092a20e4c53babf4865ac78df6/src/adapter.js#L28-L32

So I would not consider this a breaking change.

[asapach]

@Johann-S, do you still have any objections?

[Johann-S]

good catch @asapach, so LGTM 👍

[asapach]

Any chance of a new release? This has been fixed in #110

[mgol]

Ping @Johann-S?

[Johann-S]

I can cut a release on Github but I don't have npm rights 😟

/CC @dignifiedquire @Krinkle @johnjbarton

[johnjbarton]

done