Conditionally import lazy engine css #645
Conversation
|
I left a note on #644 as well, but I don't think this fix will fix my particular issue. I've updated my reproduction to include a css file -- the error still occurs in that case. |
@chriseppstein: I left a comment on the issue but this PR will still fix the new reproductions
@stefanpenner I will chat with @ef4 about this. Right now its kinda heavy handed (we have to create a new engine). I will look into if there is a lighter weight method way we can test this. |
|
@stefanpenner I updated with tests if you want to review again @ef4 This should be good to go |
| }); | ||
|
|
||
| test('lazy engine css is imported', function () { | ||
| expectFile('assets/_engine_/lazy-engine.js') |
There was a problem hiding this comment.
So, one way I test css stuff in an acceptance test Is to start the app, and inspect the relevant element's style property. Rather than parsing/matching content in output files. Is this relevant here?
The current approach indirectly tests the system, based on what appears to be build specific internals. Although this may technical work today, we have little durable confidence that the observed behavior (from a browsers perspective) is operating correctly.
There was a problem hiding this comment.
Speaking with travis, it seems this PR should add an acceptance test scenario for this case [blocker]. And (as time permits, or maybe as a follow up) adjust how we test these intermediate stage 2 outputs [non-blocker].
For example, today we are testing that a file contains a strong, but that isn't a very durable mechanism for testing.
Rather, we should execute these files in a carefully constructed environment and ensure they have the side-affects we expect. It seems like these files are not one-off, and having this machinery will enable testing all of them.
One thought would be to (with a modern version of node). Context a vmcontext that is able to execute these files.
https://nodejs.org/api/vm.html#vm_class_vm_module
Given that the vm context now has, although experimental, support for SourceTextModules creation, linking, and evaluation. see: https://nodejs.org/api/vm.html#vm_class_vm_module.
My thought here is, we invest a little in such a harness then we can execute actual tests against them. Ones who do not care about the actual source-code, but only the side-affects.
Only risk I see, is that the node API is currently unstable. If that is a concern, this could be implemented as a code-transform instead, although I would likely recommend falling back to a code-transform only if the node folks fundamentally break this api.
There was a problem hiding this comment.
This is a misunderstanding of how our tests are structured.
If you want to go end-to-end with actual execution in the browser, use one of the many test packages that run entire ember test suites. Don’t go adding VM execution to these tests, it’s fake and inferior to browser testing anyway.
Tests like the ones in stage2.test.ts are deliberately testing a static view of the build, not a runtime one.
There was a problem hiding this comment.
@ef4 the above is two thoughts, does your comment apply to both points or just the vm thought?
Specifically, are you concerned with:
Speaking with travis, it seems this PR should add an acceptance test scenario for this case [blocker].
As well as the [non-blocker] idea?
There was a problem hiding this comment.
@ef4 / @stefanpenner Should I add an additional engine that doesn't contain any styles or are we happy with theses tests
There was a problem hiding this comment.
I would be OK with adding an end-to-end test but don't consider it a blocker.
What I was trying to say in reply to @stefanpenner above is that I most definitely do not want any tests that try to evaluate browser code inside node's vmcontext. Any time someone feels tempted to do that, they should move the test into one of our many ember apps to test in the browser instead. These node tests are deliberately looking at a static view of the output, not a runtime view. It's important to have both views, because there are things you can't prove with only a runtime view (for example -- that dead code isn't present).
Fixes: #644