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

Add Support for Selenium Tests #437

Merged
merged 2 commits into from
Mar 7, 2023

Conversation

brideck
Copy link
Contributor

@brideck brideck commented Mar 1, 2023

  • Adds Selenium implementation for Faces Ajax test

- Adds Selenium implementation for Faces Ajax test
@Emily-Jiang
Copy link
Contributor

@manovotn @Ladicek can you take a look at the PR?

Copy link
Contributor

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM

@Emily-Jiang
Copy link
Contributor

Any other review comments @manovotn @Ladicek @starksm64 ? Appreciate your feedback. Thanks in advance!

@pnicolucci
Copy link

@Emily-Jiang some licensing discussion: jakartaee/faces#1794 (comment)

@pnicolucci
Copy link

@Emily-Jiang @brideck license discussion has finalized: jakartaee/faces#1794 contains the details. This PR is good to go with the Apache copyright header.

@Emily-Jiang
Copy link
Contributor

I plan to merge this PR tomorrow morning. If you have any objections, please shout asap. @starksm64 just a heads up, I will need some help from you to do a service release after this PR is merged.

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

I plan to merge this PR tomorrow morning. If you have any objections, please shout asap.

Shouldn't we instead look to remove htmlunit altogether and replace it with selenium tests?
This PR only adds a special-case execution for a singular test.

What are the requirements on impl side to execute such test?
In fact, I think there should be some documentation in terms of requirements for TCK execution and what setup is needed in the TCK doc.

@starksm64 just a heads up, I will need some help from you to do a service release after this PR is merged.

@Emily-Jiang as a committer you are able to access the Jenkins yourself and run the TCK release job followed up by the maven repo release job. I assume it's even documented somewhere but I have no idea where from the top of my head :)

@brideck
Copy link
Contributor Author

brideck commented Mar 6, 2023

Shouldn't we instead look to remove htmlunit altogether and replace it with selenium tests? This PR only adds a special-case execution for a singular test.

I don't think that a wholesale technology replacement would be an appropriate change for a TCK service release. The change is designed to provide zero impact to implementations that are already passing the TCK as-is. They would just continue to run with no changes, without adding the new property.

What are the requirements on impl side to execute such test? In fact, I think there should be some documentation in terms of requirements for TCK execution and what setup is needed in the TCK doc.

We could certainly add something to the documentation. Because the logic is the same between the two test implementations, the idea is that someone would only need run one or the other, depending on the modernity of their Faces implementation's javascript support. The changes are that someone running the Selenium tests would need to set the property in their TCK runner and also provide Chrome in their test environment.

@brideck
Copy link
Contributor Author

brideck commented Mar 6, 2023

I've added a note to the documentation explaining why and how you would want to use this new support.

@Ladicek
Copy link
Contributor

Ladicek commented Mar 7, 2023

I don't feel capable of reviewing this, but the changes seem pretty well isolated and easy to reuse / generalize / remove when we move off of HtmlUnit, so I guess I'm fine with this.

@manovotn
Copy link
Contributor

manovotn commented Mar 7, 2023

I don't think that a wholesale technology replacement would be an appropriate change for a TCK service release. The change is designed to provide zero impact to implementations that are already passing the TCK as-is. They would just continue to run with no changes, without adding the new property.

Fair enough, I've created #441 to track the future change.

I've added a note to the documentation explaining why and how you would want to use this new support.

Thanks!

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

I don't fully understand the internals of the problem but the solution is well isolated and doesn't affects other impls testing it, hence my approval :)

@Emily-Jiang Emily-Jiang merged commit 0dc76fa into jakartaee:master Mar 7, 2023
@Emily-Jiang
Copy link
Contributor

Thank you @manovotn @Ladicek for reviewing this! I will try to follow the instruction and perform a service release shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants