Skip to content

[JENKINS-64708] Deprecate PageObject constructor receiving the Injector as parameter #2003

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

akash-manna-sky
Copy link

JENKINS-64708

Now,

  1. The deprecated constructor now has a clearer warning message
  2. The new helper constructor makes it easier to migrate existing code to use contexts correctly
  3. The factory method provides a convenient migration path for existing code

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@akash-manna-sky
Copy link
Author

Hello @fcojfernandez,
could you please give me a quick initial review ? And please let me know if anything needs fixing or improving.

Thanks!

@uhafner
Copy link
Member

uhafner commented May 13, 2025

When you set an element to deprecated you should ensure that you are not using it in your own code base.

The code migration is not clear to me: All my page objects use this constructor right now. If I remember correctly, this was required to create these page objects dynamically. Is this still possible then?

@akash-manna-sky akash-manna-sky force-pushed the issue-64708 branch 6 times, most recently from ac72770 to ffe892e Compare May 15, 2025 06:23
@akash-manna-sky
Copy link
Author

Hi @uhafner,
I make few changes.
Could you please suggest if there's an alternative approach for dynamic creation under the new changes? A bit more guidance on the recommended migration path would be really helpful.

Thanks!

@uhafner
Copy link
Member

uhafner commented May 15, 2025

Hi @uhafner, I make few changes. Could you please suggest if there's an alternative approach for dynamic creation under the new changes? A bit more guidance on the recommended migration path would be really helpful.

Thanks!

I have no idea. Since you are trying to mark the constructor as deprecated I thought that you know what to do for all those plugins that use it right now. Why are you fixing this specific issue then? Or did you pick an arbitrary issue that you want to solve?

Maybe it is best to set the issue as in progress in Jira, assign your name in Jira as well and ask in Jira what should be done for those plugins that use the constructor right now. Hopefully, the author of the issue responds and tells you what to do instead.

It would be also helpful to remove the usages of this constructor in the acceptance harness. Then you will see if your proposed changes break something...

@uhafner
Copy link
Member

uhafner commented May 15, 2025

BTW: when you are rebasing your code then it makes sense to squash the commits to a single one.

@@ -53,6 +53,11 @@ protected ConfigurablePageObject(PageObject context, URL url) {
super(context, url);
}

/**
* @deprecated Use {@link #ConfigurablePageObject(PageObject, URL)} instead to properly maintain context hierarchy.
* This constructor will be removed in a future version.
Copy link
Member

Choose a reason for hiding this comment

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

that will make the Jenkins Page object interesting....

Copy link
Author

Choose a reason for hiding this comment

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

Hello @jtnord, I didn't quite get that. How do I proceed from here?
Should I try removing the references of the constructor and then see if anything breaks (like @uhafner mentioned) or there is a different approach to it?

Copy link
Member

@jtnord jtnord May 19, 2025

Choose a reason for hiding this comment

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

Hello @jtnord, I didn't quite get that.
What would you pass as the context for the Jenkins PageObject (

)

But what is the problem with the constructor with the Injector? what issue are you trying to solve here?

@akash-manna-sky akash-manna-sky force-pushed the issue-64708 branch 5 times, most recently from e9d4b43 to cb90118 Compare May 20, 2025 06:02
@jtnord jtnord requested review from jtnord and a team May 20, 2025 08:50
@basil basil self-requested a review May 20, 2025 14:44
@akash-manna-sky akash-manna-sky marked this pull request as ready for review May 20, 2025 19:32
@akash-manna-sky akash-manna-sky force-pushed the issue-64708 branch 3 times, most recently from 5d332ab to 22872d6 Compare May 21, 2025 19:17
@akash-manna-sky
Copy link
Author

Hello @basil Could you give me a quick review on how to proceed further?

Thanks!

@basil basil removed their request for review May 21, 2025 21:53
@basil
Copy link
Member

basil commented May 21, 2025

What problem is this PR trying to solve? The linked Jira ticket does not clearly describe the problem or why this task is a solution for it.

@akash-manna-sky akash-manna-sky force-pushed the issue-64708 branch 2 times, most recently from 24ef28d to 3f8f8fc Compare May 24, 2025 11:53
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.

4 participants