-
Notifications
You must be signed in to change notification settings - Fork 161
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
Override ordering for bundle dependency filters #4886
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java, line 278 at r1 (raw file):
.sorted(Comparator.comparingInt( filter -> filter instanceof BundleDependencyFilter ? 1 : 0))
This logic could be tested.
flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java, line 278 at r1 (raw file): Previously, denis-anisimov (Denis) wrote…
Please suggest any sensible way of testing this. For reference, the original implementation that was included in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java, line 278 at r1 (raw file):
Previously, Legioth (Leif Åstrand) wrote…
Please suggest any sensible way of testing this. For reference, the original implementation that was included in
master
didn't have any tests for the reasons described in #4878 (comment).
I'm not sure that I've not missed something but it looks like this should work:
- Extend
VaadinService
. - Override either
createInstantiator()
. - Return you own
Instantiator
mock. - Mock/override
getDependencyFilters
method. - Check that
getDependencyFilters()
returns theBundleDependencyFilter
instances in the end ofIterable
.
Looks like everything is doable.
Backported variant of #4878. This approach avoids API changes, but increases complexity instead of reducing it.
ab8b561
to
4ea882b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java, line 278 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
I'm not sure that I've not missed something but it looks like this should work:
- Extend
VaadinService
.- Override either
createInstantiator()
.- Return you own
Instantiator
mock.- Mock/override
getDependencyFilters
method.- Check that
getDependencyFilters()
returns theBundleDependencyFilter
instances in the end ofIterable
.Looks like everything is doable.
Done.
SonarQube analysis reported 6 issues Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained
Backported variant of #4878. This approach avoids API changes, but
increases complexity instead of reducing it.
This change is