Skip to content
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

test: Add react mixed test module #6865

Merged
merged 18 commits into from
Oct 24, 2024
Merged

Conversation

caalador
Copy link
Contributor

Add a mixed React Hilla Flow
test module to test interoperability
of the 2 features

Fixes #4972

caalador and others added 3 commits October 11, 2024 14:10
Add a mixed React Hilla Flow
test module to test interoperability
of the 2 features

Fixes #4972
Copy link

github-actions bot commented Oct 11, 2024

Dependencies Report

  • 🚫 Vulnerabilities:

  • 🟠 Known Vulnerabilities:

    • Vulnerabilities in: pkg:npm/[email protected] [CVE-2024-34394, CVE-2024-34393] (osv-scan)
      👌 This is coming from the tools, @cyclonedx/cyclonedx, we have used for sbom module, FP for us.
      ·
    • Vulnerabilities in: pkg:maven/com.fasterxml.jackson.core/[email protected] [CVE-2023-35116] (owasp)
      👌 Not a valid CVE report based on the vendor analysis and research
      · cpe:2.3:a:fasterxml:jackson-databind::::::::
    • Vulnerabilities in: pkg:maven/me.friwi/jcef-api@jcef-af53d63%2Bcef-104.4.23%2Bg46ae630%2Bchromium-104.0.5112.102 [CVE-2024-21639, CVE-2024-21640] (owasp)
      👌 Wait for the update from the jcefmaven community. Meanwhile the swing-kit is supposed to be used with fixed websites and not to browse the internet, we have a check for that, so the only possible attacker would be the same person that created the swing application, aka our customer devs. so this vulnerability is not classified by us as critical issue
      · cpe:2.3:a:chromiumembedded:chromium_embedded_framework::::::::
  • 📔 No License Issues

  • 🟠 Changes in 24.6-SNAPSHOT since V24.6.0.alpha1

    • 1 packages modified (1 external, 0 vaadin)
    • 1059 packages same (834 external, 225 vaadin)

[Click for more Details]

@caalador caalador force-pushed the issues/4972-flow-hilla-react-module branch from 08e058a to f322b92 Compare October 14, 2024 09:55
@caalador caalador marked this pull request as ready for review October 15, 2024 07:46
Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find the following test cases:

  1. Navigation lifecycle events handling on server: that event is triggered with expected number of times
  2. Server-side API for browser navigation: History::back/forward, History::pushState/replaceState

Do you think this isn't needed?

/**
* Utility function for importing style modules. This is a temporary
* solution until there is a standard solution available
* @see https://github.com/vaadin/vaadin-themable-mixin/issues/73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issues seems resolved, maybe this file is not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably can be removed. I just started with a full copy of the original hybrid module.

import com.vaadin.flow.router.RouterLink;

@Route("flow")
public class FlowMainView extends VerticalLayout {
Copy link
Contributor

@mshabarov mshabarov Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why this doesn't happen in the validation of this PR, but locally I get

Caused by: com.vaadin.flow.server.InvalidRouteConfigurationException: Invalid route configuration. The following Hilla route(s) conflict with configured Flow routes: flow

because this view has apparently the same mapping as views/flow/@index.tsx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I remove views/flow/@index.tsx, it still throws, because apparently it sees an entry with "flow" path in the file-routes.json.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it by using "flow-flow" mapping for Flow view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was the check for duplicate routes PR just a while ago and this one is older than that.
But just removing the @index.tsx from the flow folder should be enough. Probably the mapping check needs some change on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just removing it doesn't help. Perhaps we have a bug in conflicts detection in Flow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, validation passes.

@mshabarov mshabarov changed the title feat: Add react mixed test module test: Add react mixed test module Oct 23, 2024
@caalador
Copy link
Contributor Author

I'm not certain it is run as it is behind the profile. Same as the other hybrid which I don't know where it is executed.

        <profile>
            <id>react-hybrid</id>
            <modules>
                <module>vaadin-platform-react-hybrid-test</module>
            </modules>
        </profile>

@ZheSun88 any idea on the execution?

@mshabarov
Copy link
Contributor

At least yesterday I saw that these IT tests were running according to the log.

pom.xml Outdated
@@ -76,6 +76,7 @@
<module>vaadin-dev-bundle</module>
<module>vaadin-prod-bundle/pom-unoptimized.xml</module>
<module>vaadin-prod-bundle</module>
<module>vaadin-platform-react-hybrid-test</module>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test module is behind a profile and added in the standard list of modules, maybe we should excluded it from here and leave it only behind a profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@mshabarov
Copy link
Contributor

@ZheSun88 could you please add this profile to CI when appropriate?

@mshabarov mshabarov merged commit a61392e into main Oct 24, 2024
2 of 3 checks passed
@mshabarov mshabarov deleted the issues/4972-flow-hilla-react-module branch October 24, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New React hybrid application test module
3 participants