Get rid of HtmlImports in root-context ITs #7326
Conversation
project-bot
bot
added this to External Reviews
in OLD Vaadin Flow ongoing work (Vaadin 10+)
caalador
moved this from External Reviews
to Iteration Reviews
in OLD Vaadin Flow ongoing work (Vaadin 10+)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained
a discussion (no related file):
Do not merge yet: I need to take care about 3 more tests which are excluded from the build.
* Clean up remaining resources
There was a problem hiding this comment.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained
a discussion (no related file):
Previously, denis-anisimov (Denis) wrote…
Do not merge yet: I need to take care about 3 more tests which are excluded from the build.
Ready for review
There was a problem hiding this comment.
I think it would be helpful if we add a comment about why we have 3 different eager-module.js files in test-root-context.
Reviewed 103 of 109 files at r1, 26 of 27 files at r2.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @mehdi-vaadin)
flow-tests/test-root-context/frontend/eager-module.js, line 9 at r2 (raw file):
if (window.messages)
Should we add && window.messages.forEach here as well?
flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/CustomCustomElement.js, line 1 at r2 (raw file):
// A custom element that isn't using Polymer
CustomCustomElement.js is referenced in CustomCustomElementView and should be removed.
flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/dependencies/ContextInlineApiView.java, line 32 at r2 (raw file):
frontend-inline.css
This file is missing.
There was a problem hiding this comment.
I think there is only two eager-module.js.
And we have two eager.js, eager-module.css, etc.
And it's clear for Java code why, because: one is used from annotation and has to be in the frontend folder,
another one is loaded via API and has to be in the web resources folder.
Do you have any idea how it may be clarified ?
Reviewed 1 of 27 files at r2.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @mehdi-vaadin)
flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/CustomCustomElement.js, line 1 at r2 (raw file):
Previously, mehdi-vaadin (Mehdi Javan) wrote…
CustomCustomElement.jsis referenced inCustomCustomElementViewand should be removed.
CustomCustomElementView is not removed .
It has @JsModule("com/vaadin/flow/uitest/ui/CustomCustomElement.js").
And it has test for it.
But it also contained incorrect JS with frontend schema. That I've removed.
flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/dependencies/ContextInlineApiView.java, line 32 at r2 (raw file):
Previously, mehdi-vaadin (Mehdi Javan) wrote…
frontend-inline.cssThis file is missing.
Done.
There was a problem hiding this comment.
I think there is only two eager-module.js.
I can see three.
- flow-tests/test-root-context/frontend/eager-module.js
- flow-tests/test-root-context/frontend/dependencies/eager-module.js
- flow-tests/test-root-context/src/main/webapp/dependencies/eager-module.js
Maybe the one in test-root-context/frontend/dependencies can be removed.
Do you have any idea how it may be clarified?
I have no idea and that's why I think it needs clarifying. It looks complicated to me. It seems to me that the eager-module.js that is added in DependenciesLoadingPageApiView doesn't work independently and depends on the one that is added via an annotation in DependenciesLoadingAnnotationsView. I didn't get why there is this dependency.
One more thing is that it seems that AnnotatedContextInlineView uses a css file (frontend-inline.css) which has been removed.
Reviewed 1 of 27 files at r2, 3 of 3 files at r3.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained
There was a problem hiding this comment.
Right, there are three files. One is made accidentally by mistake.
It's removed.
AnnotatedContextInlineView is fixed.
About clarification:
- if you go from Java side then it's clear where the files are located. For annotations it's
frontendfolder, for API methods it's web resources. I don't like this inconsistency in fact but this is out of scope of this PR. eager-module.jsdepends oneager.jsindeed. But this is an existing test which I'm trying to adapt to NPM mode where HtmlImport doesn't exist and replaced by the JsModule. That's a direct replacement. It's true that the test is not obvious . But clarification of every IT is not the purpose of this PR.
It's OK to make a ticket to clarify the test. Would be good if it had been done initially by the author of this test.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained (waiting on @mehdi-vaadin)
There was a problem hiding this comment.
Reviewed 3 of 4 files at r4.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
I created #7348. |
Part of #7242
This change is